feat: Allow custom options in the RunConfiguration - #2590
feat: Allow custom options in the RunConfiguration#2590cristianonicolai wants to merge 1 commit into
Conversation
e4a0e9d to
630af29
Compare
630af29 to
98aa11a
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the service API model to support passing arbitrary custom key/value options as part of a RunConfiguration, and exposes that capability through the generated OpenAPI schema.
Changes:
- Add
options: Map<String, String>toRunConfiguration, including constructor overloads and propagation throughwithTermination(...)/override(...). - Update the generated OpenAPI schema for
RunConfigurationto include the newoptionsobject property.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| service/test-model/src/build/openapi.json | Adds options map property to the RunConfiguration schema. |
| service/definition/src/main/java/ai/timefold/solver/service/definition/api/domain/RunConfiguration.java | Adds options field to the record and threads it through copy/override behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
98aa11a to
c6e016d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
service/definition/src/main/java/ai/timefold/solver/service/definition/api/domain/RunConfiguration.java:94
- The tag override guard uses
!finalTags.isEmpty()which prevents inheriting tags when the current tags are empty (the default from constructors) and can unintentionally overwrite non-empty tags, so it should checkfinalTags.isEmpty()and assign via the accessor.
if ((finalTags == null || !finalTags.isEmpty()) && configuration.tags() != null && !configuration.tags().isEmpty()) {
finalTags = configuration.tags;
}
service/definition/src/main/java/ai/timefold/solver/service/definition/api/domain/RunConfiguration.java:26
- Marking the new
optionsproperty as@Schema(hidden = true)while the record schema forbids additional properties (@Schema(additionalProperties = Schema.False.class)) makes the OpenAPI contract disallow the very field this PR adds, so either expose it in the schema or revisit the schema-level additionalProperties policy.
@JsonInclude(JsonInclude.Include.NON_NULL) @Schema(hidden = true) Map<String, String> options) {
c6e016d to
2e83b23
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
service/definition/src/main/java/ai/timefold/solver/service/definition/api/domain/RunConfiguration.java:25
- The new options field is marked @Schema(hidden = true), which contradicts the stated goal of allowing custom options because it will be omitted from generated OpenAPI/SDKs.
@JsonInclude(JsonInclude.Include.NON_NULL) @Schema(
description = "Optional tags to be assigned to the dataset.") @Size(max = 100) Set<String> tags,
@JsonInclude(JsonInclude.Include.NON_NULL) @Schema(hidden = true) Map<String, String> options) {
service/definition/src/main/java/ai/timefold/solver/service/definition/api/domain/RunConfiguration.java:15
- Unlike other API domain records (for example ModelRequest.java:11), RunConfiguration no longer declares @Schema(additionalProperties = Schema.False.class), which can mislead OpenAPI clients into sending unknown fields that Jackson rejects (FAIL_ON_UNKNOWN_PROPERTIES), so re-add the annotation and keep custom data under the explicit options property.
public record RunConfiguration(
diogodanielsoaresferreira
left a comment
There was a problem hiding this comment.
Looks good to me 👍
|
I would appreciate if the Sonar/Copilot comments were resolved before we merge this. If the comment is wrong, it's fine to close it without any action - but very often, these comments are right. |
|
@triceo I dont see much option here on what could be done to satisfy it both ways. Idea is that |
|
I am more in favour of allowing additional properties rather than make the field visible to all. The whole idea with this was to not promote this to end consumers. |
2e83b23 to
9194ebe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
service/definition/src/main/java/ai/timefold/solver/service/definition/api/domain/RunConfiguration.java:84
- The condition "(finalTags == null || !finalTags.isEmpty())" in override() overwrites existing non-empty tags and fails to inherit tags when the current set is empty; change it to check for emptiness and assign via the accessor.
if ((finalTags == null || !finalTags.isEmpty()) && configuration.tags() != null && !configuration.tags().isEmpty()) {
finalTags = configuration.tags;
}
service/definition/src/main/java/ai/timefold/solver/service/definition/api/domain/RunConfiguration.java:16
- Dropping @Schema(additionalProperties = Schema.False.class) from RunConfiguration makes the OpenAPI schema accept arbitrary unknown properties even though the service ObjectMapper is configured to fail on unknown properties, leading to surprising request failures unless schema and deserialization are aligned.
public record RunConfiguration(
@Schema(nullable = true,
9194ebe to
35a3605
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
service/definition/src/main/java/ai/timefold/solver/service/definition/api/domain/RunConfiguration.java:25
- Removing @Schema(additionalProperties = Schema.False.class) from RunConfiguration makes the OpenAPI schema accept arbitrary unknown run configuration fields (not just the intended options), which can desync client-side/schema validation from the strict Jackson mapping; consider keeping additionalProperties=false and exposing a documented options map property instead of hiding it (or otherwise constraining schema permissiveness).
public record RunConfiguration(
@Schema(nullable = true,
description = "Optional name to be given to the dataset. If not provided, the name will be generated.") @Size(
min = 0, max = 255) String name,
@JsonInclude(JsonInclude.Include.NON_NULL) SolverTerminationConfig termination,
@Schema(nullable = true,
description = "Optional maximum number of threads to be used for solving.",
minimum = "1") @JsonInclude(JsonInclude.Include.NON_EMPTY) @Positive Integer maxThreadCount,
@JsonInclude(JsonInclude.Include.NON_NULL) @Schema(
description = "Optional tags to be assigned to the dataset.") @Size(max = 100) Set<String> tags,
@JsonInclude(JsonInclude.Include.NON_NULL) @Schema(hidden = true) Map<String, String> options) {
| @JsonInclude(JsonInclude.Include.NON_NULL) @Schema( | ||
| description = "Optional tags to be assigned to the dataset.") @Size(max = 100) Set<String> tags) { | ||
| description = "Optional tags to be assigned to the dataset.") @Size(max = 100) Set<String> tags, | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) @Schema(hidden = true) Map<String, String> options) { |
There was a problem hiding this comment.
If it's hidden from the schema, it cannot be sent from the client, since the platform gateway will consider it an invalid parameter, correct?
So we allow these options to only be sent inside the cluster.
There was a problem hiding this comment.
the idea is that it can still be part of the POST body, so it wont fail, but only the Platform would know how to handle it. Sort of hidden parameter to be used.
There was a problem hiding this comment.
I have slightly different view, shall we have a short meeting tomorrow or on Monday three of us (@cristianonicolai @rsynek )?
| @JsonInclude(JsonInclude.Include.NON_NULL) @Schema( | ||
| description = "Optional tags to be assigned to the dataset.") @Size(max = 100) Set<String> tags) { | ||
| description = "Optional tags to be assigned to the dataset.") @Size(max = 100) Set<String> tags, | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) @Schema(hidden = true) Map<String, String> options) { |
There was a problem hiding this comment.
Do we want to keep it open for any parameters?
If we want to configure a custom seed, it will probably originate from the platform configuration profile, but we will need to set quarkus.timefold.solver.random-seed={some-unknown-key} to propagate it to the solver.
If we make change it to a Map<InternalOptions, String>, where InternalOptions is an enum, we have a good control over what we send to the model pods as hidden options.
There was a problem hiding this comment.
Idea was to keep it open to allow for other customization in the future. Other options that wouldnt be public available. I think if we add a InternalOptions enum, it would still show up in the OpenAPI as type no?
Regarding the quarkus.timefold.solver.random-seed that would be set as env var by the Platform, at least thats how I understood this need to be done. So despite changing the RunConfiguration here, thats only because we need to extend it to accept these other parameters. Do you see another way this should be done?
Related to https://github.com/TimefoldAI/timefold-platform/issues/5335