gerlowskija commented on PR #2788:
URL: https://github.com/apache/solr/pull/2788#issuecomment-2438568772
Still going through the individual files on this PR, but wanted to respond
to some of the high-level comments first:
> Wasn't sure how to model the response. Different APIs use different error
for "does not exist" responses: /api/collections/collectionName returns a 400,
/api/aliases/specificalias returns a 405,
/solr/collectionName/schema/fields/fieldName returns a 404
I'm personally a fan of 404 in this case as it seems a little more
actionable for users than the more generic '400', so that'd be my preference.
But I don't have any strong feelings on that point, and would be open to
something else if you do have preferences? When we decide, we should document
the decision in `dev-docs/v2-api-conventions.adoc` so there's a "standard" we
can align on.
(I suspect the 405 returned by `GET /api/aliases/nonexistentAlias` is a bug,
FWIW. Will have to file a ticket for that if I can reproduce...)
> I wasn't sure if that would be preferred over grouping them all together
as was done with AliasPropertyApis/AliasProperty
I prefer grouping related APIs into a single file, at least on the 'api'
side. IMO it cuts down on boilerplate, and makes reviewing and browsing easier
by keeping a bunch of related definitions together. But again, it's a very
slight preference on my end if you happen to prefer the alternative.
> The new v2 JAX-RS Bulk Update ClusterProp API requires providing a body
that looks like {"properties":{"actualPropertyToBeUpdated":...}} because I
didn't know how to map an unknown top-level value
Hmm - I think you should be able to nuke
`SetNestedClusterPropertyRequestBody` altogether, and replace it in the method
signature with `Map<String, Object>`? e.g.
```
@PUT
@Operation(
summary = "Set nested cluster properties in this Solr cluster",
tags = {"cluster-properties"})
SolrJerseyResponse createOrUpdateNestedClusterProperty(
@RequestBody(description = "Property/ies to be set", required = true)
Map<String, Object> propertyValuesByName)
```
Or does that break something or other that I've forgotten about?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
