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

Decouple OpenApiAnnotationScanner from Jackson #245

Closed
tjquinno opened this issue Jan 28, 2020 · 10 comments · Fixed by #250
Closed

Decouple OpenApiAnnotationScanner from Jackson #245

tjquinno opened this issue Jan 28, 2020 · 10 comments · Fixed by #250
Labels
enhancement New feature or request

Comments

@tjquinno
Copy link
Contributor

tjquinno commented Jan 28, 2020

We are layering on SmallRye's OpenAPI implementation as much as we can but want to use a parser other than Jackson.

We do our own parsing of any static file to create an OpenAPI instance, rather than using OpenApiProcessor#modelFromStaticFile, and we do our own serializing.

But OpenApiAnnotationScanner depends on Jackson's ObjectMapper...I think in two places inside #parseExtension.

I don't see an easy way to continue to use OpenApiAnnotationScanner as-is without also relying on Jackson.

Questions:

  1. Am I missing an easy solution?
  2. Is there any chance you could decouple OpenApiAnnotationScanner from Jackson's ObjectMapper and allow the caller to provide a way to accomplish the equivalent?
  3. I know there are some other references to Jackson classes from elsewhere in SmallRye - e.g., IgnoreResolver, JsonUtil, and a few others - but I'm not sure they are in the code paths we'd continue to rely on.

Thanks.

@EricWittmann
Copy link
Contributor

I think that seems like a pretty reasonable ask.

@MikeEdgar
Copy link
Member

Hi @tjquinno - which parser are you looking at? I'm wondering if it might be a better approach to have #parseExtension return a javax.json.JsonValue to stick with standard APIs. If it needs to remain an Object, we could go with a configurable interface. Is that what you're thinking @EricWittmann ?

@tjquinno
Copy link
Contributor Author

We happen to be using SnakeYAML, but that shouldn't matter.

@MikeEdgar
Copy link
Member

As this seems like more of a feature to be used only by a vendor rather than an application, I'm considering adding a method to AnnotationScannerExtension with Jackson being the default implementation. @tjquinno , there is an existing OpenApiAnnotationScanner constructor for you to pass in a list of scanner extensions which would include an implementation with your parser.

I think the default implementation also needs to be modified to return plain Java types (List or Map) instead of Jackson's JsonNode. That would be in alignment with the static file reader, but does have the potential for breaking a user's OASFilter.

Thoughts?

@EricWittmann
Copy link
Contributor

Good thinking about the filter - although I wonder how many users are filtering at all, let alone filtering on extensions. :) Still, you're right that a change there could break filtering.

@MikeEdgar
Copy link
Member

I'm leaning toward just changing it. The likelihood of a developer specifying extensions via annotations that need to be filtered in the same application seems extremely low :)

@tjquinno
Copy link
Contributor Author

Mike, I am aware of the extensions mechanism and that seems like a reasonable way to expose the behavior we're talking about.

We'd just want to be sure that the default implementation you referred to that would use Jackson would not require Jackson to be present at runtime if, in fact, that default implementation were never used.

@MikeEdgar
Copy link
Member

@tjquinno - we're on the same page. We'll need to have a unit test or tests for this that ensure Jackson is not on the class path.

@tjquinno
Copy link
Contributor Author

@MikeEdgar Just for planning purposes on my end... Are you thinking of introducing this change in a few days, a few weeks, a few months...?

Thanks.

@MikeEdgar
Copy link
Member

It should not be more than a few days. The challenge I'm facing now is having multiple executions of the unit tests (one with Jackson and one without) that works with both the Maven command line as well as with the IDE tooling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants