Skip to content

Commit 7057674

Browse files
jknackjknack
authored
Merge pull request #3984 from kliushnichenko/fix/stable-schema-props-order
build: make schema properties order stable
2 parents 45d6975 + 073d69d commit 7057674

4 files changed

Lines changed: 203 additions & 0 deletions

File tree

‎modules/jooby-openapi/src/main/java/io/jooby/internal/openapi/ParserContext.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,7 @@ public Schema schema(Class type) {
274274
new SchemaRef(
275275
resolvedSchema.schema, RefUtils.constructRef(resolvedSchema.schema.getName()));
276276
schemas.put(type.getName(), schemaRef);
277+
stabilizeProperties(type, resolvedSchema.schema);
277278
document(type, resolvedSchema.schema, resolvedSchema);
278279
if (resolvedSchema.referencedSchemas != null) {
279280
for (var e : resolvedSchema.referencedSchemas.entrySet()) {
@@ -286,6 +287,7 @@ public Schema schema(Class type) {
286287
for (var e : resolvedSchema.referencedSchemasByType.entrySet()) {
287288
var qualifiedTypeName = toClass(e.getKey());
288289
if (qualifiedTypeName instanceof Class<?> classType) {
290+
stabilizeProperties(classType, e.getValue());
289291
document(classType, e.getValue(), resolvedSchema);
290292
}
291293
}
@@ -304,6 +306,14 @@ private java.lang.reflect.Type toClass(java.lang.reflect.Type type) {
304306
return type;
305307
}
306308

309+
private void stabilizeProperties(Class<?> type, Schema schema) {
310+
if (schema == null || schema.getProperties() == null || schema.getProperties().isEmpty()) {
311+
return;
312+
}
313+
var node = classNodeOrNull(Type.getType(type));
314+
SchemaPropertyOrder.stabilize(node, schema, this::classNodeOrNull);
315+
}
316+
307317
private void document(Class typeName, Schema schema, ResolvedSchemaExt resolvedSchema) {
308318
javadocParser
309319
.parse(typeName.getName())

‎modules/jooby-openapi/src/main/java/io/jooby/internal/openapi/SchemaPropertyOrder.java‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
/*
2+
* Jooby https://jooby.io
3+
* Apache License Version 2.0 https://jooby.io/LICENSE.txt
4+
* Copyright 2014 Edgar Espina
5+
*/
6+
package io.jooby.internal.openapi;
7+
8+
import java.util.ArrayList;
9+
import java.util.LinkedHashMap;
10+
import java.util.LinkedHashSet;
11+
import java.util.List;
12+
import java.util.Map;
13+
import java.util.Set;
14+
import java.util.function.Function;
15+
16+
import org.objectweb.asm.Opcodes;
17+
import org.objectweb.asm.Type;
18+
import org.objectweb.asm.tree.ClassNode;
19+
import org.objectweb.asm.tree.FieldNode;
20+
import org.objectweb.asm.tree.MethodNode;
21+
22+
import io.swagger.v3.oas.models.media.Schema;
23+
24+
/**
25+
* Makes {@link Schema#getProperties()} order deterministic by reordering keys according to
26+
* class-file declaration order (fields, then getters), including the superclass chain.
27+
*
28+
* <p>This preserves a natural bean-like order
29+
*/
30+
public final class SchemaPropertyOrder {
31+
32+
private SchemaPropertyOrder() {}
33+
34+
public static void stabilize(
35+
ClassNode node, Schema<?> schema, Function<Type, ClassNode> classNodes) {
36+
Map<String, Schema> properties = schema.getProperties();
37+
if (properties == null || properties.isEmpty() || node == null) {
38+
return;
39+
}
40+
List<String> declarationOrder = declarationOrder(node, classNodes);
41+
if (declarationOrder.isEmpty()) {
42+
return;
43+
}
44+
45+
var ordered = new LinkedHashMap<String, Schema>();
46+
for (String name : declarationOrder) {
47+
Schema property = properties.get(name);
48+
if (property != null) {
49+
ordered.put(name, property);
50+
}
51+
}
52+
// Keep any leftover properties (e.g. synthetic names) in their original relative order.
53+
properties.forEach(ordered::putIfAbsent);
54+
schema.setProperties(ordered);
55+
}
56+
57+
static List<String> declarationOrder(ClassNode node, Function<Type, ClassNode> classNodes) {
58+
var names = new LinkedHashSet<String>();
59+
collect(node, classNodes, names);
60+
return new ArrayList<>(names);
61+
}
62+
63+
private static void collect(
64+
ClassNode node, Function<Type, ClassNode> classNodes, Set<String> names) {
65+
if (node == null || isExcluded(node.name)) {
66+
return;
67+
}
68+
if (node.superName != null && !isExcluded(node.superName)) {
69+
collect(classNodes.apply(Type.getObjectType(node.superName)), classNodes, names);
70+
}
71+
if (node.fields != null) {
72+
for (FieldNode field : node.fields) {
73+
if (isInstanceField(field)) {
74+
names.add(field.name);
75+
}
76+
}
77+
}
78+
if (node.methods != null) {
79+
for (MethodNode method : node.methods) {
80+
if (isGetter(method)) {
81+
names.add(propertyName(method.name));
82+
}
83+
}
84+
}
85+
}
86+
87+
private static boolean isExcluded(String internalName) {
88+
return internalName == null
89+
|| internalName.equals("java/lang/Object")
90+
|| internalName.equals("java/lang/Record")
91+
|| internalName.equals("java/lang/Enum");
92+
}
93+
94+
private static boolean isInstanceField(FieldNode field) {
95+
return (field.access & Opcodes.ACC_STATIC) == 0
96+
&& (field.access & Opcodes.ACC_SYNTHETIC) == 0;
97+
}
98+
99+
private static boolean isGetter(MethodNode method) {
100+
if ((method.access & Opcodes.ACC_STATIC) != 0
101+
|| (method.access & Opcodes.ACC_PUBLIC) == 0
102+
|| (method.access & Opcodes.ACC_SYNTHETIC) != 0
103+
|| (method.access & Opcodes.ACC_BRIDGE) != 0) {
104+
return false;
105+
}
106+
if (Type.getArgumentTypes(method.desc).length != 0) {
107+
return false;
108+
}
109+
Type returnType = Type.getReturnType(method.desc);
110+
if (returnType.equals(Type.VOID_TYPE)) {
111+
return false;
112+
}
113+
if (method.name.startsWith("get") && method.name.length() > 3) {
114+
return true;
115+
}
116+
return method.name.startsWith("is")
117+
&& method.name.length() > 2
118+
&& (returnType.equals(Type.BOOLEAN_TYPE)
119+
|| returnType.getClassName().equals(Boolean.class.getName()));
120+
}
121+
122+
private static String propertyName(String methodName) {
123+
if (methodName.startsWith("get")) {
124+
return decapitalize(methodName.substring(3));
125+
}
126+
if (methodName.startsWith("is")) {
127+
return decapitalize(methodName.substring(2));
128+
}
129+
return methodName;
130+
}
131+
132+
/** Same rules as {@code java.beans.Introspector.decapitalize}. */
133+
private static String decapitalize(String name) {
134+
if (name == null || name.isEmpty()) {
135+
return name;
136+
}
137+
if (name.length() > 1
138+
&& Character.isUpperCase(name.charAt(0))
139+
&& Character.isUpperCase(name.charAt(1))) {
140+
return name;
141+
}
142+
char[] chars = name.toCharArray();
143+
chars[0] = Character.toLowerCase(chars[0]);
144+
return new String(chars);
145+
}
146+
}

‎modules/jooby-openapi/src/test/java/examples/MediaTypeSchemaApp.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/*
2+
* Jooby https://jooby.io
3+
* Apache License Version 2.0 https://jooby.io/LICENSE.txt
4+
* Copyright 2014 Edgar Espina
5+
*/
6+
package examples;
7+
8+
import io.jooby.Context;
9+
import io.jooby.Jooby;
10+
import io.jooby.MediaType;
11+
12+
public class MediaTypeSchemaApp extends Jooby {
13+
{
14+
get("/media-type", this::mediaType);
15+
}
16+
17+
public MediaType mediaType(Context ctx) {
18+
return MediaType.json;
19+
}
20+
}

‎modules/jooby-openapi/src/test/java/io/jooby/openapi/OpenAPIGeneratorTest.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,16 @@
1111
import static org.junit.jupiter.api.Assertions.assertNull;
1212
import static org.junit.jupiter.api.Assertions.assertTrue;
1313

14+
import java.util.ArrayList;
1415
import java.util.Arrays;
16+
import java.util.List;
1517
import java.util.Map;
1618
import java.util.concurrent.Callable;
1719

1820
import com.fasterxml.jackson.databind.JavaType;
1921
import examples.ABean;
2022
import examples.Letter;
23+
import examples.MediaTypeSchemaApp;
2124
import examples.MvcApp;
2225
import examples.MvcAppWithRoutes;
2326
import examples.MvcInstanceApp;
@@ -33,6 +36,7 @@
3336
import examples.RouteQueryArgs;
3437
import examples.RouteReturnTypeApp;
3538
import examples.RouterProduceConsume;
39+
import io.jooby.internal.openapi.OpenAPIExt;
3640
import io.jooby.internal.openapi.RequestBodyExt;
3741
import io.swagger.v3.oas.models.media.ArraySchema;
3842
import io.swagger.v3.oas.models.media.BooleanSchema;
@@ -1636,4 +1640,27 @@ public void ktAppWithMain(RouteIterator iterator) {
16361640
})
16371641
.verify();
16381642
}
1643+
1644+
/**
1645+
* Check that the schema property order is stable across repeated OpenAPI
1646+
* generations.
1647+
*
1648+
* <p>For example, the static field {@code MediaType.json} and boolean getter {@code isJson()}
1649+
* resolve to the same OpenAPI property name {@code json}. Without stabilizing property order from
1650+
* class-file declaration order, {@code json} (and other boolean properties like {@code textual})
1651+
* can jump between builds.
1652+
*/
1653+
@SuppressWarnings({"unchecked", "rawtypes"})
1654+
@OpenAPITest(MediaTypeSchemaApp.class)
1655+
public void mediaTypePropertiesOrderIsReproducible(OpenAPIExt openApi) {
1656+
var mediaType = openApi.getComponents().getSchemas().get("MediaType");
1657+
1658+
assertNotNull(mediaType);
1659+
1660+
// Declaration order: instance fields (charset, value), then getters (quality, textual, json,
1661+
// type, subtype). Static MediaType.json must not displace isJson()'s property.
1662+
assertEquals(
1663+
List.of("charset", "value", "quality", "textual", "json", "type", "subtype"),
1664+
new ArrayList<>(mediaType.getProperties().keySet()));
1665+
}
16391666
}

0 commit comments

Comments
 (0)