SOLR-17326: Fix wrong references in generated API classes

[malliaridis]

https://issues.apache.org/jira/browse/SOLR-17326

Description

Adds a test and fixes SOLR-17326 by using the model package in affected class references.

Solution

In the java-template/api.mustache the modelPackage is used to add the packge to returnType references, so that in case of a naming conflict, the correct class is still used.

Tests

The new test uses an existing generated response class that references the wrong class in the return type. A dummy response is created and parsed, then the class of the parsed data is checked. During the test, if a conflict is present, parsing errors may occur if the data contains unknown fields that are not ignored.

Checklist

Please review the following and check all that apply:

• I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• I have created a Jira issue and added the issue ID to my pull request title.
• I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
• I have developed this patch against the main branch.
• I have run ./gradlew check.
• I have added tests for my changes.
• I have added documentation for the Reference Guide

Additional Information

It is possible to add a generics bound in JacksonParsingResponse<T> that limits T to only classes that extend SolrJerseyResponse, but this will limit the possible classes for T. This would ensure potential result in compile errors if t

With the use of fully qualified names in the current solution, some imports in the generated classes are redundant but cannot be excluded due to the way they are added. The solution implemented here is not perfect, as it cannot be guaranteed that the return type class is always in the model package. The same applies for the test, as it cannot be guaranteed that in future there won't be any classes that do not extend SolrJerseyResponse. However, in the current state of the project, all API return types extend SolrJerseyResponse.

Alternative slutions that were considered:

1. Rename the API endpoint return types

This solution would solve the naming conflicts, but cannot guarantee future conflicts without additional "guards" / checks. Additionaly, this would introduce changes in the OpenAPI and API module that users may depend on.

2. Limit the allowed types in JacksonParsingResponse<T> with a generics upper bound

This option would introduce a guard that would break compilation, identifying conflicts early on. However, this would require additional actions (e.g. option 1) to fix existing conflicts. Additionally, return types of newly added endpoints would always have to inherit from SolrJerseyResponse (which is currently the case and therefore checked in the added test).

[gerlowskija]

Hey @malliaridis - thanks for the fix, and thanks even more for the detailed writeup. I wish everyone (myself included) was so thorough in listing out other potential approaches. This is awesome!

Ultimately I think the approach you took here is the best one available for now.

As you pointed out, it does have the downside that it requires all response classes to live in the same package. I agree this is non-ideal, but something we can live with. In fact, there are actually one or other two places where the template relies on the "single package" assumption. I tried hard to avoid it in the initial implementation, but couldn't crack it entirely at the time. In hindsight I think we could probably do something based on vendor-extension annotations (like the one used in SelectApi), but that'd require a larger effort than just fixing this bug.

It is possible to add a generics bound in JacksonParsingResponse that limits T to only classes that extend SolrJerseyResponse, but this will limit the possible classes for T

I think this would probably be OK as a longer-term solution. Though I wonder about a few APIs that return "blobs" of data (e.g. returning a single file out of a configset). These responses are "just" raw data, so they don't quite fit into the SolrJerseyResponse model which assumes at least a bare minimum of structure (e.g. responseHeader). That feels like a solve-able problem, but it might take some thought...

[gerlowskija]

This LGTM!

@malliaridis - do you mind if I take this out of "Draft" mode and start working towards merge? I'll likely make a few minor changes (add a CHANGES.txt entry to give you credit for the fix, tweak the test Javadoc), but otherwise this looks ready for a committer to get involved and get it closed out.

[malliaridis]

Thanks for the positive feedback, it means a lot.

Though I wonder about a few APIs that return "blobs" of data

If this could be addressed then adding additional type-safety would definitely be something for looking forward to.

do you mind if I take this out of "Draft" mode and start working towards merge

Go ahead and thank you for taking over. :)

[gerlowskija]

Thanks for the fix @malliaridis !

(FYI - I've filed SOLR-17329 to address the type-narrowing you suggested adding to JacksonParsingResponse. So we can take care of that at some point too!)