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

Create an initial version of the JSON schema processors #8

Merged
merged 42 commits into from Apr 23, 2024

Conversation

andriy-dmytruk
Copy link
Collaborator

Based on the proposal in this issue: #2.

The implementation includes a visitor that visits types that require a JSON schema and generates the files during build time.

Global configuration is available with the JsonSchemaConfiguration annotation.

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

Can we support the generation of a JSON Schema of an enum?

e.g. https://github.com/getmanfred/mac/blob/master/schema/schema.json#L1281-L1290

gradle/libs.versions.toml Outdated Show resolved Hide resolved
gradle/libs.versions.toml Outdated Show resolved Hide resolved
gradle/libs.versions.toml Outdated Show resolved Hide resolved
gradle/libs.versions.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

I think for serving schemas is best to recommend users to use the Static Resources support as we do for OpenAPI instead of creating a controller.

@andriy-dmytruk
Copy link
Collaborator Author

I think for serving schemas is best to recommend users to use the Static Resources support as we do for OpenAPI instead of creating a controller.

I tried this using it, but there is a small problem. The schema URL (id) generally omits the .schema.json extension: https://json-schema.org/understanding-json-schema/structuring#id. The controller has a minor change of adding the extension if needed. If we could somehow solve this with static resources, that would be better, though.

@andriy-dmytruk
Copy link
Collaborator Author

Other changes look good. Thanks, Sergio!

@sdelamo
Copy link
Contributor

sdelamo commented Apr 8, 2024

Other changes look good. Thanks, Sergio!

Yes, I have another PR in progress. I wanted to send it today. But I will send it tomorrow.

@andriy-dmytruk
Copy link
Collaborator Author

@sdelamo Do we want to merge this now and send PRs for new fixes/features to master? At this point the PR is pretty large

return topLevelSchema;
}

private static void setSchemaType(TypedElement element, VisitorContext visitorContext, JsonSchemaContext context, Schema schema) {

Choose a reason for hiding this comment

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

we probably need a way for the developer to manually specify the type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but I don't think type would be enough if users would need to customize the schema. We probably need ability to customize other things, too. Other projects don't seem to offer customizing this field directly: https://github.com/FasterXML/jackson-module-jsonSchema, https://github.com/mbknor/mbknor-jackson-jsonSchema. Although the second repository supports @JsonSchemaOptions annotations which allows setting any property of the generated schema.

include::src/main/docs/guide/annotationProcessorOption.html[]
++++

<1> Set the base URL for all the schemas. It will be prepended to all relative schema URLs.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like this now:
image
image

Since the baseUri is an important option, it would seem strange to not mention how it can be specified. I probably spent like an hour finding out how annotation processing options are specified for different languages and build tools, so others are likely to have the same issue if we do not mention it.

@graemerocher Perhaps we should support a better way of configuring annotation processing? Like a processing.properties file that gets read from resources. It sounds strange, but at least it would provide a unique way of setting the options.

Choose a reason for hiding this comment

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

this seems like a common challenge that both the openapi module and this module probably share, it is probably possible to make it simpler in Gradle not sure if it is possible in Maven

//cc @melix @alvarosanchez

Choose a reason for hiding this comment

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

btw shouldn't the Maven config prefix be -A not -

Copy link
Member

Choose a reason for hiding this comment

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

It is -A, yes.

What exactly is expected to be done with Maven?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What exactly is expected to be done with Maven?

Somehow load the annotation processor options from a properties file or somewhere else so that there would be a unified way to configure them for both gradle and maven.

Copy link
Member

Choose a reason for hiding this comment

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

In Maven, it's the Maven Compiler Plugin the one that compiles, and this is how you pass annotation processing parameters: https://maven.apache.org/plugins/maven-compiler-plugin/examples/pass-compiler-arguments.html

Theoretically and hypothetically, we could create a Maven extension that, before compilation, would monkey patch the Maven Compiler Plugin configuration XML at runtime if a processing.properties file is found. It is a can of worms inside a box of snakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking we can add options in here: https://github.com/micronaut-projects/micronaut-core/blob/a04877b58f5c4e98b91ffec498fa48b25c68785a/inject-java/src/main/java/io/micronaut/annotation/processing/visitor/JavaVisitorContext.java#L441-L449.

It could probably be something like:

FileObject fileObject = processingEnvironment.getFiler()
    .getResource( StandardLocation.CLASS_OUTPUT, "", "processing.properties" );
InputStream jsonStream = fileObject.openInputStream();

which relies on the fact that resources should be copied to the build folder. But I guess current GroovyVisitorContext won't be able to do this. And I am not sure if the behavior would be consistent for Maven & Groovy, so yeah, it's complicated.

Copy link

Choose a reason for hiding this comment

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

issue: I don't think we should do this. If a file shows up in the output classes folder, then it will end in the user application, where it has nothing to do. I think this should be solved on the build plugins side. There are several options for Gradle, which are not mutually exclusive:

  1. add a generic option under the processing block of the micronaut configuration block to pass parameters
  2. (preferred) use a strongly-typed DSL for json-schema, which would automatically populate the annotation processor parameters. This option has the benefit that if options of the processor are, say, input files, then we can make them part of inputs and behave correctly wrt up-to-date checking, which is not the case with stringly typed arguments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @melix. I guess my main issue with this is that to update an option will require changes to 3 repositories: this repository, gradle plugin repository and maven plugin repository. And the documentation would also be in different places, too.

I agree that other solutions seem inferior, so I suppose this is what I will have to do.

Copy link

sonarcloud bot commented Apr 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
7.5% Coverage on New Code (required ≥ 70%)
1 New Bugs (required ≤ 0)
1 New Blocker Issues (required ≤ 0)
6 New Critical Issues (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@andriy-dmytruk andriy-dmytruk marked this pull request as ready for review April 19, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants