Skip to content

Commit

Permalink
fix(repository): don't generate invalid grammar (#2833)
Browse files Browse the repository at this point in the history
When performing a search with a pipeline name that doesn't exist
sqlrepository will generate a query with an empty IN clause:
```
SELECT * FROM ... WHERE IN ()
```

this change short circuits this evaluation.

Additionally, set H2 into `MODE=MYSQL` so that we can actually catch
this invalid SQL in unit tests
  • Loading branch information
marchello2000 authored Apr 15, 2019
1 parent a572ba2 commit 0442a08
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.config.TransactionRetryProperties
import com.netflix.spinnaker.kork.core.RetrySupport
import com.netflix.spinnaker.orca.ExecutionStatus
import com.netflix.spinnaker.orca.ExecutionStatus.BUFFERED
import com.netflix.spinnaker.orca.ExecutionStatus.CANCELED
import com.netflix.spinnaker.orca.ExecutionStatus.NOT_STARTED
import com.netflix.spinnaker.orca.ExecutionStatus.PAUSED
import com.netflix.spinnaker.orca.ExecutionStatus.RUNNING
import com.netflix.spinnaker.orca.ExecutionStatus.*
import com.netflix.spinnaker.orca.pipeline.model.Execution
import com.netflix.spinnaker.orca.pipeline.model.Execution.ExecutionType
import com.netflix.spinnaker.orca.pipeline.model.Execution.ExecutionType.ORCHESTRATION
Expand All @@ -33,28 +29,15 @@ import com.netflix.spinnaker.orca.pipeline.model.Stage
import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionNotFoundException
import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository
import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionComparator
import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionComparator.BUILD_TIME_DESC
import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionComparator.NATURAL_ASC
import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionComparator.START_TIME_OR_ID
import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionComparator.*
import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionCriteria
import com.netflix.spinnaker.orca.pipeline.persistence.UnpausablePipelineException
import com.netflix.spinnaker.orca.pipeline.persistence.UnresumablePipelineException
import de.huxhorn.sulky.ulid.SpinULID
import org.jooq.DSLContext
import org.jooq.Field
import org.jooq.Record
import org.jooq.SelectConditionStep
import org.jooq.SelectConnectByStep
import org.jooq.SelectForUpdateStep
import org.jooq.SelectJoinStep
import org.jooq.SelectWhereStep
import org.jooq.Table
import org.jooq.*
import org.jooq.exception.SQLDialectNotSupportedException
import org.jooq.impl.DSL
import org.jooq.impl.DSL.count
import org.jooq.impl.DSL.field
import org.jooq.impl.DSL.name
import org.jooq.impl.DSL.table
import org.jooq.impl.DSL.*
import org.slf4j.LoggerFactory
import rx.Observable
import java.lang.System.currentTimeMillis
Expand Down Expand Up @@ -443,14 +426,17 @@ class SqlExecutionRepository(
val select = jooq.selectExecutions(
PIPELINE,
conditions = {
val inClause = "config_id IN (${pipelineConfigIds.joinToString(",") { "'$it'" }})"
val timeCondition = it.where(inClause)
var conditions = it.where(
field("config_id").`in`(*pipelineConfigIds.toTypedArray())
.and(field("build_time").gt(buildTimeStartBoundary))
.and(field("build_time").lt(buildTimeEndBoundary))
var conditions = timeCondition
)

if (executionCriteria.statuses.isNotEmpty()) {
conditions = timeCondition.and("status IN (${executionCriteria.statuses.joinToString(",") { "'$it'" }})")
val statusStrings = executionCriteria.statuses.map { it.toString() }
conditions = conditions.and(field("status").`in`(*statusStrings.toTypedArray()))
}

conditions
},
seek = {
Expand Down Expand Up @@ -693,7 +679,9 @@ class SqlExecutionRepository(
if (statuses.isEmpty() || statuses.size == ExecutionStatus.values().size) {
return this
}
val clause = "status IN (${statuses.joinToString(",") { "'$it'" }})"

var statusStrings = statuses.map { it.toString() }
val clause = DSL.field("status").`in`(*statusStrings.toTypedArray())

return run {
when (this) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -491,4 +491,14 @@ class SqlExecutionRepositorySpec extends ExecutionRepositoryTck<SqlExecutionRepo
storeLimit = 6
}

def "doesn't fail on empty configIds"() {
expect:
repository
.retrieveAllPipelinesForPipelineConfigIdsBetweenBuildTimeBoundary(
[],
0L,
5L,
new ExecutionCriteria().setPageSize(1).setSortType(BUILD_TIME_ASC)
).size() == 0
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,17 @@
import java.sql.SQLException;
import java.util.Arrays;

import static org.jooq.SQLDialect.H2;
import static org.jooq.SQLDialect.MYSQL;
import static org.jooq.conf.RenderNameStyle.AS_IS;

public class SqlTestUtil {

public static TestDatabase initDatabase() {
return initDatabase("jdbc:h2:mem:orca_test");
return initDatabase("jdbc:h2:mem:orca_test;MODE=MYSQL");
}

public static TestDatabase initPreviousDatabase() {
return initDatabase("jdbc:h2:mem:orca_test_previous");
return initDatabase("jdbc:h2:mem:orca_test_previous;MODE=MYSQL");
}

public static TestDatabase initDatabase(String jdbcUrl) {
Expand All @@ -54,7 +54,7 @@ public static TestDatabase initDatabase(String jdbcUrl) {

DefaultConfiguration config = new DefaultConfiguration();
config.set(new DataSourceConnectionProvider(dataSource));
config.setSQLDialect(H2);
config.setSQLDialect(MYSQL);
config.settings().withRenderNameStyle(AS_IS);

DSLContext context = new DefaultDSLContext(config);
Expand Down

0 comments on commit 0442a08

Please sign in to comment.