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

JacksonModule bug? #491

Closed
rdykiel opened this issue Oct 30, 2024 · 10 comments · Fixed by #492
Closed

JacksonModule bug? #491

rdykiel opened this issue Oct 30, 2024 · 10 comments · Fixed by #492

Comments

@rdykiel
Copy link

rdykiel commented Oct 30, 2024

I have a Java object model structured as follows:

public class DeploymentType {
...
    @JsonProperty("import")
    @XmlElement(name = "import")
    protected ImportType _import;
...

Using plain jsonschema I can generate a JSON schema where the 'deployment' object has the '_import' field. Note that the JsonProperty renaming the field to 'import' is ignored.

I then tried to use a JacksonModule in order to interpret Jackson annotations like JsonProperty and JsonPropertyDescription. The resulting JSON schemas are all missing the '_import' field. All the other fields are properly included, even the other one that is renamed via a JsonProperty. I verified that the JsonProperty and JsonPropertyDescription were properly rendered.

I don't know what it is with that '_import' property and after trying all the workarounds I could think of (including renaming '_import' to 'importers') I am opening this ticket.

Pending a resolution I would welcome any advice on how to debug this since I have downloaded the code. Thanks.

@CarstenWickner
Copy link
Member

Hi @rdykiel,

A complete example would help in debugging – including your JSON Schema Generator configuration.
I suspect the leading underscore is the issue.

What does the getter look like? get_import() or getImport()?
Any particular reason for the leading underscore there? Could it be removed?
When you use the jackson ObjectMapper to serialize such a DeploymentType instance, does it include the import property?

@rdykiel
Copy link
Author

rdykiel commented Oct 30, 2024

I'll ask my manager about how much code I can communicate, or will work on a repro.
The underscore isn't the issue AFAIK because I tried renaming it to 'importers' and had the same result.
The underscore comes from the fact that you can't have a field 'protected ImportType _import;' in a Java class definition, import being a reserved name?

The getter is:
    public ImportType getImport() {
        return _import;
    }

The DeploymentType class is parsed from YAML and generates YAML correctly ('_import' being renamed 'import' by the effect of the JsonProperty annotation), with the standard Jackson libraries.

Here is the code of the schema generation:

    public String getSchemaGenSchema() {
        try {
            JacksonModule module = new JacksonModule();
            SchemaGeneratorConfigBuilder configBuilder = new SchemaGeneratorConfigBuilder(SchemaVersion.DRAFT_2020_12, OptionPreset.PLAIN_JSON);
            SchemaGeneratorConfig config = configBuilder
                    .with(module)
                    .with(Option.EXTRA_OPEN_API_FORMAT_VALUES)
                    .without(Option.FLATTENED_ENUMS_FROM_TOSTRING)
                    .build();

            SchemaGenerator generator = new SchemaGenerator(config);
            JsonNode jsonSchema = generator.generateSchema(YamlRoot.class);

            ObjectMapper mapper = configBuilder.getObjectMapper();
            builder.append(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(jsonSchema));
            builder.append("\n");
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
        return builder.toString();
    }

The class 'YamlRoot' is a simple class that only has 1 field, 'DeploymentType deployment'. This is how we generate the desired YAML and parse it.

@rdykiel
Copy link
Author

rdykiel commented Oct 30, 2024

I want to restate that when not using the JacksonModule, I get the '_import' section in the output JSON schema correctly (of course the JsonProperty renaming doesn't occur)

@CarstenWickner
Copy link
Member

Hi @rdykiel

I suspect changing the getter name to get_import() would do the trick.
In the JacksonModule I'm asking Jackson whether a given property should be visible or not. For such a field without a conventional getter (same for the combination importers and getImport()), I suspect the answer is "No".

@rdykiel
Copy link
Author

rdykiel commented Oct 31, 2024

I created 'get_import' and 'get__import' and this didn't change the behavior (I kept the existing getImport() for compatibility with other code). I'll continue investigating.

@CarstenWickner
Copy link
Member

Hi @rdykiel,

As you were asking for the place to debug:

return beanDescription.findProperties().stream()
.noneMatch(propertyDefinition -> fieldName.equals(propertyDefinition.getInternalName()));

By default, Jackson will pick up the import property based on the getImport() getter and ignore the _import field because it has the conflicting @JsonProperty("import") annotation.
Currently, the JacksonModule compares the field's declared name _import with the getter's internal name which happens to be import.

A workaround would probably be through the Option.FIELDS_DERIVED_FROM_ARGUMENTFREE_METHODS and Option.NONSTATIC_NONVOID_NONGETTER_METHODS options being enabled on the generator, but I agree with your initial assessment that this seems to be a bug in the JacksonModule that is worth fixing.

@rdykiel
Copy link
Author

rdykiel commented Oct 31, 2024

Thanks for your analysis. I need to divert myself to an emergency so won't be able to work on this project for several days. And we have even some more time after that before this becomes a blocking issue for us.

@CarstenWickner
Copy link
Member

I've made the required fix.
Before releasing I'm still trying to solve another request though. As I understand there is a bit of time before you need the fix.

@rdykiel
Copy link
Author

rdykiel commented Nov 1, 2024

Wonderful! Yes there is no rush. LMK when the new release is available? Thanks for the quick turnaround.

@CarstenWickner
Copy link
Member

Release v4.37.0 has just been published containing the aforementioned fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants