-
Notifications
You must be signed in to change notification settings - Fork 251
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
data-management-api: validate query parameters #1258
data-management-api: validate query parameters #1258
Conversation
...nsions/api/api-core/src/main/java/org/eclipse/dataspaceconnector/api/query/QuerySpecDto.java
Outdated
Show resolved
Hide resolved
return sortField; | ||
} | ||
|
||
@AssertTrue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to have the @JsonIgnore
annotation, otherwise Jackson will look for a valid
field. A serialization test would have surfaced that. Mind adding one?
For example:
@Test
void verifySerialization() throws JsonProcessingException {
var dto = QuerySpecDto.Builder.newInstance()
.filter("some filter")
.limit(10)
.offset(2)
.build();
var mapper = new ObjectMapper();
var json = mapper.writeValueAsString(dto);
assertThat(json).isNotNull();
var d = mapper.readValue(json, QuerySpecDto.class);
assertThat(d).usingRecursiveComparison().isEqualTo(dto);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is never serialized to/deserialized from json, so it's not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked, but what about tests (integration, e2e)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests are working just fine without touching them, I just add a "failing query" test for every controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, that'll work, but as far as I'm aware @algattik has plans to generate a java API client from our own OpenAPI spec and use that in tests. If that is the case we'd indeed have to serialize the QuerySpecDto
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paullatzelsperger I'm not sure about that, however I would leave this for that issue, if it's needed it will be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fine for me.
Hi @ndr-brt Will the approach you introduce in this PR also applicable in the context of the Data Plane API? |
hey @bscholtes1A I don't think the same approach could be used there because, as you said, there isn't the possibility to know what parameters will be passed upfront. |
What this PR changes/adds
Adds validation on query params before building the
QuerySpec
object.To do that introduces a
QuerySpecDto
object that is valued through the@BeanParam
annotation.Why it does that
To better validate input on query call
Further notes
Linked Issue(s)
Closes #1235
Checklist
no-changelog
)