Skip to content

Conversation

@imretoth-ess
Copy link
Contributor

Extended swagger documentation for the REST interface with response objects and status-codes

@jacomago
Copy link
Contributor

Is it possible to replace all the MultiValueMap with more specific list of query params? Most of them I think are for the search and it should be possible to have query params like:

~size
~name
~total_hits
MultiVAlue
tag

@github-actions
Copy link

Overall Project 1.23%
File Coverage
ChannelProcessorManager.java 0% 🍏
TagManager.java 0% 🍏
PropertyManager.java 0% 🍏
InfoManager.java 0% 🍏
ChannelScroll.java 0% 🍏
ChannelManager.java 0% 🍏

@imretoth-ess
Copy link
Contributor Author

Is it possible to replace all the MultiValueMap with more specific list of query params? Most of them I think are for the search and it should be possible to have query params like:

~size ~name ~total_hits MultiVAlue tag

Well, yes and no.
The MultiValueMap key can end with a '!' character which would mean negation for some of that field when querying. In that case either the '!' character should move to the value part of the MultiValueMap, or introduce an additional boolean flag that would mark the negation per queryParam fields. Either one we choose would change the current syntax - if we remove those fields from being parsed when processing the MultiValueMap.

Another thing I just noticed that if the RequestBody is an Iterable type, it is not showing up on swagger. I think it can not decide what structure it should show/parse, since List, Map,... have different structure.

Copy link
Contributor

@anderslindho anderslindho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Largely looks good to me, but some nitpicks here and there.

@jacomago I don't think we will want to create a release with annotating the operations... Should we create follow-up or handle here? It might 2x the PR and work.

* @param allRequestParams query parameters
* @return list of all channels
*/
@ApiResponses(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add @Operation annotations here and elsewhere

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ignore this part @imretoth-ess - I will create a sub-task and I can take that myself so you don't have to modify the scope in the middle of the PR

responseCode = "200",
description = "The number of matches for the query, and the first 10k channels",
content = @Content(
array = @ArraySchema(schema = @Schema(implementation = SearchResult.class)))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, doesn't this return a SearchResult object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it responds with a SearchResult object that contains only 2 fields: a count, and a List of Channels - but on the response List length there is a restriction

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacomago thoughts?

/**
* PUT method for creating multiple channels.
*
* @param channels - XmlChannels to be created
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically out of scope for your PR but this isn't right - both XmlChannels and "to be created" (as it's PUT)

@sonarqubecloud
Copy link

@github-actions
Copy link

Overall Project 1.23%
File Coverage
ChannelProcessorManager.java 0% 🍏
TagManager.java 0% 🍏
PropertyManager.java 0% 🍏
InfoManager.java 0% 🍏
ChannelScroll.java 0% 🍏
ChannelManager.java 0% 🍏

@anderslindho anderslindho self-requested a review May 28, 2025 14:00
@anderslindho
Copy link
Contributor

LGTM

@jacomago jacomago merged commit bcc1b4d into master Jun 2, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants