Skip to content
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

SOLR-16825: Migrate API definitions to submodule, pt 2 #1866

Merged

Conversation

gerlowskija
Copy link
Contributor

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

Description

SOLR-16825 added a new gradle module, 'api', which holds v2 API definitions as interfaces. This allows us to generate an OAS (and SolrRequest implementations from that) as a part of the solrj build.

But these artifacts (the OAS and generated Java code), only cover the v2 APIs that have interfaces in the 'api' module. We need to extract interfaces to live in 'api' for each v2 API in 'core' that doesn't already have one.

Solution

This PR creates 'api' interfaces for a number of v2 APIs, allowing SolrRequest implementations to be generated for them. The following APIs are covered in this PR:

  • list-aliases
  • list-incremental-backups
  • list-collections
  • list-configsets
  • reload-collection
  • various alias-property CRUD APIs

Tests

Manual inspection of the generated OAS and SolrRequest implementations.

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.

Mustache templating is designed to generate simple HTML pages, and so by
default it escapes input variables as necessary to exist in a HTML
document.

This, combined with the '<' and '>' characters used in Java generics,
causes problems for us in code generation.  To avoid this, this commit
updates our template to use the "triple-brace" syntax in a number of
places, which tells mustache to not do any special escaping of the
templated values.
@HoustonPutman
Copy link
Contributor

Is it possible to make the model classes static inner classes of the interfaces that use them?

@gerlowskija
Copy link
Contributor Author

gerlowskija commented Aug 31, 2023

Kindof?

That approach works fine as far as JAX-RS and Solr are concerned. In fact I'm sure you noticed it's how things were structured when these classes lived in 'core'.

The problem is code-generation. As each SolrRequest class is being generated, it needs to be able to import the Java POJOs it relies on. And the SolrRequest-generation is using the OAS as its input, not the existing Java code directly - so it doesn't have access to information like Java fully-qualified class names as it's being processed.

(openapi-generator has the ability to generate model POJOs for users, and handles imports much more gracefully in this case. We've decided not to generate models though, since they'd essentially duplicate the POJOs that we already have. The "import" problem is the main downside of that decision.)

Right now we work around this limitation in our template by keeping things pretty simple in terms of imports - we essentially expect all POJOs to be in a single package without any static class nesting that'd change the import pattern. (see here and here)

Anyway, this feels like a fixable problem to me if it's worth it to us to dig in. I spent a few hours tweaking the OpenAPI spec and api.mustache template without much success, but there's gotta be some fix out there. I guess it just depends how strongly we care about being able to have POJOs nested the way @HoustonPutman described...

@HoustonPutman
Copy link
Contributor

Thanks for the explanation! In general it seems a little daunting to have them all separated and there, when it would be easier to work with them in the same place. Maybe it'll be worth it to look into sometime. Maybe it won't... But I'm not going to block you on it here.

@gerlowskija
Copy link
Contributor Author

Sounds good; I'll merge this in for now and we can revisit later if needed.

@gerlowskija gerlowskija merged commit 9426902 into apache:main Sep 1, 2023
3 checks passed
@gerlowskija gerlowskija deleted the SOLR-16825-api-def-migration-pt2 branch September 1, 2023 17:07
gerlowskija added a commit that referenced this pull request Sep 1, 2023
Extracting annotated interfaces for these APIs includes them in the SolrRequest-
generation we now do in SolrJ.
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.

2 participants