Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling build id in view page #651

Merged
merged 8 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion src/main/groovy/io/seqera/wave/controller/ViewController.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@

package io.seqera.wave.controller

import java.util.regex.Pattern
import groovy.json.JsonOutput
import io.micronaut.core.annotation.Nullable

import groovy.transform.CompileStatic
import io.micronaut.context.annotation.Value
import io.micronaut.core.annotation.Nullable
import io.micronaut.http.HttpResponse
import io.micronaut.http.annotation.Controller
import io.micronaut.http.annotation.Get
Expand Down Expand Up @@ -70,13 +72,47 @@ class ViewController {

@View("build-view")
@Get('/builds/{buildId}')
HttpResponse<Map<String,String>> viewBuild(String buildId) {
HttpResponse viewBuild(String buildId) {
// check redirection for invalid suffix in the form `-nn`
final r1 = shouldRedirect1(buildId)
if( r1 )
return HttpResponse.redirect(URI.create(r1))
// check redirection when missing the suffix `_nn`
final r2 = shouldRedirect2(buildId)
if( r2 )
return HttpResponse.redirect(URI.create(r2))
// go ahead with proper handling
final record = buildService.getBuildRecord(buildId)
if( !record )
throw new NotFoundException("Unknown build id '$buildId'")
return HttpResponse.ok(renderBuildView(record))
}

static final private Pattern DASH_SUFFIX = ~/([0-9a-zA-Z\-]+)-(\d+)$/

static final private Pattern MISSING_SUFFIX = ~/([0-9a-zA-Z\-]+)(?<!_\d{2})$/

protected String shouldRedirect1(String buildId) {
// check for build id containing a -nn suffix
final check1 = DASH_SUFFIX.matcher(buildId)
if( check1.matches() ) {
return "/view/builds/${check1.group(1)}_${check1.group(2)}"
}
return null
}

protected String shouldRedirect2(String buildId) {
// check build id missing the _nn suffix
if( !MISSING_SUFFIX.matcher(buildId).matches() )
return null

final rec = buildService.getLatestBuild(buildId)
if( !rec || !rec.buildId.startsWith(buildId) )
return null

return "/view/builds/${rec.buildId}"
}

Map<String,String> renderBuildView(WaveBuildRecord result) {
// create template binding
final binding = new HashMap(20)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import static io.seqera.wave.util.StringUtils.trunc
@CompileStatic
class BuildRequest {

static final String SEP = '_'
static final public String SEP = '_'

/**
* Unique request Id. This is computed as a consistent hash generated from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,13 @@ interface ContainerBuildService {
* @return The {@link WaveBuildRecord} associated with the corresponding Id, or {@code null} if it cannot be found
*/
WaveBuildRecord getBuildRecord(String buildId)

/**
* Retrieve the latest build record available for the specified container id.
*
* @param containerId The ID of the container for which the build record needs to be retrieve
* @return The {@link WaveBuildRecord} associated with the corresponding Id, or {@code null} if it cannot be found
*/
WaveBuildRecord getLatestBuild(String containerId)

}
Original file line number Diff line number Diff line change
Expand Up @@ -445,4 +445,8 @@ class ContainerBuildServiceImpl implements ContainerBuildService, JobHandler<Bui
return buildRecordStore.getBuildRecord(buildId) ?: persistenceService.loadBuild(buildId)
}

@Override
WaveBuildRecord getLatestBuild(String containerId) {
return persistenceService.latestBuild(containerId)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ interface PersistenceService {
*/
WaveBuildRecord loadBuild(String targetImage, String digest)

/**
* Retrieve the latest {@link WaveBuildRecord} object for the given container id
*
* @param containerId The container id for which the latest build record should be retrieved
* @return The corresponding {@link WaveBuildRecord} object or {@code null} if no record is found
*/
WaveBuildRecord latestBuild(String containerId)

/**
* Store a {@link WaveContainerRecord} object in the Surreal wave_request table.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ class LocalPersistenceService implements PersistenceService {
return buildStore.get(buildId)
}

@Override
WaveBuildRecord latestBuild(String containerId) {
buildStore
.values()
.findAll( it-> it.buildId.startsWith(containerId) )
.sort( it-> it.startTime )
.reverse() [0]
}

@Override
WaveBuildRecord loadBuild(String targetImage, String digest) {
buildStore.values().find( (build) -> build.targetImage==targetImage && build.digest==digest )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,16 @@ class SurrealPersistenceService implements PersistenceService {
return result
}

@Override
WaveBuildRecord latestBuild(String containerId) {
final query = "select * from wave_build where buildId ~ '${containerId}${BuildRequest.SEP}' order by startTime desc limit 1"
pditommaso marked this conversation as resolved.
Show resolved Hide resolved
final json = surrealDb.sqlAsString(getAuthorization(), query)
final type = new TypeReference<ArrayList<SurrealResult<WaveBuildRecord>>>() {}
final data= json ? JacksonHelper.fromJson(json, type) : null
final result = data && data[0].result ? data[0].result[0] : null
return result
}

@Override
void saveContainerRequest(String token, WaveContainerRecord data) {
surrealDb.insertContainerRequestAsync(authorization, token, data).subscribe({ result->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package io.seqera.wave.controller

import spock.lang.Specification
import spock.lang.Unroll

import java.time.Duration
import java.time.Instant
Expand All @@ -35,6 +36,7 @@ import io.seqera.wave.core.ContainerPlatform
import io.seqera.wave.core.spec.ContainerSpec
import io.seqera.wave.exception.DockerRegistryException
import io.seqera.wave.service.ContainerRequestData
import io.seqera.wave.service.builder.ContainerBuildService
import io.seqera.wave.service.inspect.ContainerInspectService
import io.seqera.wave.service.logs.BuildLogService
import io.seqera.wave.service.logs.BuildLogServiceImpl
Expand Down Expand Up @@ -367,4 +369,43 @@ class ViewControllerTest extends Specification {
binding.build_url == 'http://foo.com/view/builds/12345'
}

@Unroll
def 'should validate redirection check' () {
given:
def service = Mock(ContainerBuildService)
def controller = new ViewController(buildService: service)

when:
def result = controller.shouldRedirect1(BUILD)
then:
result == EXPECTED

where:
BUILD | EXPECTED
'12345_1' | null
'12345-1' | '/view/builds/12345_1'
'foo-887766-1' | '/view/builds/foo-887766_1'

}


def 'should validate redirect 2' () {
given:
def service = Mock(ContainerBuildService)
def controller = new ViewController(buildService: service)

when:
def result = controller.shouldRedirect2(BUILD)
then:
result == EXPECTED
TIMES * service.getLatestBuild(BUILD) >> LATEST

where:
BUILD | TIMES | LATEST | EXPECTED
'12345_1' | 0 | null | null
'12345' | 1 | Mock(WaveBuildRecord) { buildId >> '12345_99' } | '/view/builds/12345_99'
'12345' | 1 | Mock(WaveBuildRecord) { buildId >> 'xyz_99' } | null
'foo-887766' | 1 | Mock(WaveBuildRecord) { buildId >> 'foo-887766_99' } | '/view/builds/foo-887766_99'
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,46 @@ class SurrealPersistenceServiceTest extends Specification implements SurrealDBTe
loaded == record
}

def 'should find latest build' () {
given:
def persistence = applicationContext.getBean(SurrealPersistenceService)
final request = new BuildRequest(
'abc',
'FROM foo:latest',
'conda::recipe',
null,
Path.of("."),
'docker.io/my/repo:container1234',
PlatformId.NULL,
ContainerPlatform.of('amd64'),
'docker.io/my/cache',
'127.0.0.1',
'{"config":"json"}',
null,
null,
'scan12345',
null,
BuildFormat.DOCKER,
Duration.ofMinutes(1) )


def result = new BuildResult(request.withBuildId('1').buildId, -1, "ok", Instant.now().minusSeconds(3), Duration.ofSeconds(2), null)
persistence.saveBuild(WaveBuildRecord.fromEvent(new BuildEvent(request, result)))
and:
result = new BuildResult(request.withBuildId('2').buildId, -1, "ok", Instant.now().minusSeconds(2), Duration.ofSeconds(2), null)
persistence.saveBuild(WaveBuildRecord.fromEvent(new BuildEvent(request, result)))
and:
result = new BuildResult(request.withBuildId('3').buildId, -1, "ok", Instant.now().minusSeconds(1), Duration.ofSeconds(2), null)
persistence.saveBuild(WaveBuildRecord.fromEvent(new BuildEvent(request, result)))

when:
sleep 200
def loaded = persistence.latestBuild('abc')

then:
loaded.buildId == 'abc_3'
}

def 'should save and update a build' () {
given:
def persistence = applicationContext.getBean(SurrealPersistenceService)
Expand Down
Loading