Skip to content

Commit

Permalink
fix: Jackson JsonPropertySorter handling non-getter methods (#424)
Browse files Browse the repository at this point in the history
* fix: JsonPropertyOrder with child annotations (#423)

* chore: clean up test

* chore(docs): update CHANGELOG

* chore(docs): update contributors list

* chore: harmonise indentations

---------

Co-authored-by: Daniel Gómez-Sánchez <7352559+magicDGS@users.noreply.github.com>
  • Loading branch information
CarstenWickner and magicDGS committed Dec 18, 2023
1 parent 25e9c5d commit f0550bb
Show file tree
Hide file tree
Showing 6 changed files with 130 additions and 8 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
*-*
### `jsonschema-module-jackson`
#### Fixed
- Respect `@JsonPropertyOrder` also for properties derived from non-getter methods

## [4.33.0] - 2023-11-23
### `jsonschema-generator`
Expand Down
1 change: 1 addition & 0 deletions jsonschema-generator-parent/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@
<url>https://github.com/magicDGS</url>
<roles>
<role>Provided PR #300 (introducing support for standard "format" values via Option)</role>
<role>Provided PR #423 (fixing Jackson property order handling)</role>
</roles>
</contributor>
</contributors>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ private Set<String> collectTextItemsFromArrayNode(JsonNode arrayNode) {
* @return supplier of the successfully merged schemas (into a new node) or {@code null} if merging the given nodes is not easily possible
*/
private Supplier<ObjectNode> mergeSchemas(ObjectNode mainNodeIncludingAllOf, List<JsonNode> nodes, Map<String, SchemaKeyword> reverseKeywordMap) {
if (nodes.stream().anyMatch(part -> !(part instanceof ObjectNode) && !(part.isBoolean() && part.asBoolean()))) {
if (nodes.stream().anyMatch(part -> part.isBoolean() && !part.asBoolean())) {
return null;
}
List<ObjectNode> parts = nodes.stream()
Expand All @@ -354,9 +354,7 @@ private Supplier<ObjectNode> mergeSchemas(ObjectNode mainNodeIncludingAllOf, Lis
.collect(Collectors.toList());

// collect all defined attributes from the separate parts and check whether there are incompatible differences
Map<String, List<JsonNode>> fieldsFromAllParts = parts.stream()
.flatMap(part -> StreamSupport.stream(((Iterable<Map.Entry<String, JsonNode>>) part::fields).spliterator(), false))
.collect(Collectors.groupingBy(Map.Entry::getKey, LinkedHashMap::new, Collectors.mapping(Map.Entry::getValue, Collectors.toList())));
Map<String, List<JsonNode>> fieldsFromAllParts = this.getFieldsFromAllParts(parts);
if (this.shouldSkipMergingAllOf(mainNodeIncludingAllOf, parts, fieldsFromAllParts)) {
return null;
}
Expand Down Expand Up @@ -384,13 +382,25 @@ private Supplier<ObjectNode> mergeSchemas(ObjectNode mainNodeIncludingAllOf, Lis
};
}

/**
* Collect all defined attributes from the separate parts.
*
* @param parts entries of the {@link SchemaKeyword#TAG_ALLOF} array to consider
* @return flattened collection of all attributes in the given parts
*/
private Map<String, List<JsonNode>> getFieldsFromAllParts(List<ObjectNode> parts) {
return parts.stream()
.flatMap(part -> StreamSupport.stream(((Iterable<Map.Entry<String, JsonNode>>) part::fields).spliterator(), false))
.collect(Collectors.groupingBy(Map.Entry::getKey, LinkedHashMap::new, Collectors.mapping(Map.Entry::getValue, Collectors.toList())));
}

/**
* Check whether the merging of the given node and it's allOf entries should be skipped due to a {@link SchemaKeyword#TAG_REF} being present.
* Drafts 6 and 7 would ignore any other attributes besides the {@link SchemaKeyword#TAG_REF}.
*
* @param mainNode the main node containing an {@link SchemaKeyword#TAG_ALLOF} array (maybe {@code null})
* @param parts entries of the {@link SchemaKeyword#TAG_ALLOF} array to consider
* @param fieldsFromAllParts flatten collection of all attributes in the given parts
* @param fieldsFromAllParts flattened collection of all attributes in the given parts
* @return whether to block merging of the given {@link SchemaKeyword#TAG_ALLOF} candidate
*/
private boolean shouldSkipMergingAllOf(ObjectNode mainNode, List<ObjectNode> parts, Map<String, List<JsonNode>> fieldsFromAllParts) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ protected int getPropertyIndex(MemberScope<?, ?> property) {
.computeIfAbsent(topMostHierarchyType.getErasedType(), this::getAnnotatedPropertyOrder);
String fieldName;
if (property instanceof MethodScope) {
fieldName = Optional.ofNullable(((MethodScope) property).findGetterField()).map(MemberScope::getSchemaPropertyName).orElse(null);
fieldName = Optional.<MemberScope>ofNullable(((MethodScope) property).findGetterField())
// since 4.33.1: fall-back on method's property name if no getter can be found
.orElse(property)
.getSchemaPropertyName();
} else {
fieldName = property.getSchemaPropertyName();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ private static String loadResource(String resourcePath) throws IOException {
}
}
return stringBuilder.toString();

}

@JsonClassDescription("test description")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* Copyright 2023 VicTools.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.github.victools.jsonschema.module.jackson;

import com.fasterxml.jackson.databind.node.ObjectNode;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonPropertyOrder;
import com.fasterxml.jackson.databind.JsonNode;
import com.github.victools.jsonschema.generator.Option;
import com.github.victools.jsonschema.generator.OptionPreset;
import com.github.victools.jsonschema.generator.SchemaGenerator;
import com.github.victools.jsonschema.generator.SchemaGeneratorConfig;
import com.github.victools.jsonschema.generator.SchemaGeneratorConfigBuilder;
import com.github.victools.jsonschema.generator.SchemaKeyword;
import com.github.victools.jsonschema.generator.SchemaVersion;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Stream;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

public class JsonPropertySorterIntegrationTest {

@ParameterizedTest
@CsvSource({
"TestObject, one two three",
"TestContainer, one two three"
})
public void testJsonPropertyOrderWithChildAnnotations(String targetTypeName, String expectedFieldOrder) throws Exception {
JacksonModule module = new JacksonModule(JacksonOption.RESPECT_JSONPROPERTY_ORDER,
JacksonOption.INCLUDE_ONLY_JSONPROPERTY_ANNOTATED_METHODS);
SchemaGeneratorConfig config = new SchemaGeneratorConfigBuilder(SchemaVersion.DRAFT_2019_09, OptionPreset.PLAIN_JSON)
.with(Option.NONSTATIC_NONVOID_NONGETTER_METHODS, Option.FIELDS_DERIVED_FROM_ARGUMENTFREE_METHODS)
.with(module)
.build();
Class<?> targetType = Stream.of(JsonPropertySorterIntegrationTest.class.getDeclaredClasses())
.filter(testType -> testType.getSimpleName().equals(targetTypeName))
.findFirst()
.orElseThrow(IllegalArgumentException::new);
SchemaGenerator generator = new SchemaGenerator(config);
JsonNode result = generator.generateSchema(targetType);

ObjectNode properties = (ObjectNode) result.get(config.getKeyword(SchemaKeyword.TAG_PROPERTIES));
List<String> resultPropertyNames = new ArrayList<>();
properties.fieldNames().forEachRemaining(resultPropertyNames::add);
Assertions.assertEquals(Arrays.asList(expectedFieldOrder.split(" ")), resultPropertyNames);
}

@JsonPropertyOrder({"one", "two", "three"})
public static class TestContainer {

@JsonProperty("three")
private Integer thirdInteger;
@JsonProperty("one")
private String firstString;

@JsonProperty("two")
public boolean getSecondValue() {
return true;
}
}

@JsonPropertyOrder({"one", "two", "three"})
public static class TestObject {

private TestContainer container;
private String secondString;

@JsonIgnore
public TestContainer getContainer() {
return this.container;
}

@JsonProperty("three")
public Integer getInteger() {
return this.container.thirdInteger;
}

@JsonProperty("two")
public String getSecondString() {
return this.secondString;
}

@JsonProperty("one")
public String getString() {
return this.container.firstString;
}
}

}

0 comments on commit f0550bb

Please sign in to comment.