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

Support the "format" string tag without the other OpenAPI format-values #396

Closed
magicDGS opened this issue Nov 6, 2023 · 5 comments · Fixed by #399
Closed

Support the "format" string tag without the other OpenAPI format-values #396

magicDGS opened this issue Nov 6, 2023 · 5 comments · Fixed by #399

Comments

@magicDGS
Copy link
Contributor

magicDGS commented Nov 6, 2023

When looking into how to get the "format" tag properly formatted following the specs https://json-schema.org/understanding-json-schema/reference/string#format, I found that the only supported option for that is the Option.EXTRA_OPEN_API_FORMAT_VALUES. Nevertheless, this introduces"format" tags also for properties of numeric types that are not defined in the json-schema specs.

I know that there is a way to implement this with a customized method, but it would be nice to have support with an Option to use the supported built-in formats (https://json-schema.org/understanding-json-schema/reference/string#built-in-formats) from the json-schema specs (without relying on third-party OpenAPI custom formats).

Is there a possibility to support that feature out-of-the-box with either an option or directly when a version schema supports that built-in format?

Reproduction: I realized when I tried to use the 2019-09 schema with a model containing an UUID and an Integer, expecting to either get a "format": uuid with some option (i.e., Option.ADDITIONAL_FIXED_TYPES) but the only way was providing the OpenAPI format values that are also adding the "format": int32 to the number and integer fields.

@CarstenWickner
Copy link
Member

Hi @magicDGS,

The Option.EXTRA_OPEN_API_FORMAT_VALUES is realized through the SimpleTypeModule.
That currently contains these:

  • Integer, int = "int32"
  • Long, long = "int64"
  • Double, double = "double"
  • Float, float = "float"

With Option.ADDITIONAL_FIXED_TYPES, you'd also include:

  • java.time.LocalDate, "date"
  • java.time.LocalDateTime, java.time.ZonedDateTime, java.time.OffsetDateTime, java.time.Instant, ´java.util.Date`, ´java.util.Calendar´ = "date-time"
  • java.time.LocalTime, java.time.OffsetTime = "time"
  • ´java.util.UUID` = "uuid"
  • `java.net.URI´, = "URI"

I understand you'd like to see the "date"/"date-time"/"time"/"uuid"/"uri" formats being always applied for the respective types, without the extra non-JSON-schema-standard OpenAPI format values (e.g., ´"int32"`).

Long story short: one must know all possible data types for which a particular format is desired.
The standard list here would be very short or possibly contested by other users. Plus: this would be limited to standard Java types and you may have to take care of custom types yourself anyway.

Therefore, my preference so far is to recommend this being done through configuration.
One option being the `withStringFormatResolver()´ you already mentioned:

Map<Class<?>, String> typesWithFormat = Map.of(
    UUID.class, "uuid", URI.class, "uri", LocalDateTime.class, "date-time"
);
configBuilder.forTypesInGeneral()
    .withStringFormatResolver(scope -> typesWithFormat.get(scope.getType().getErasedType()));

However, this is only meaningful for types resolved as "type": "string", i.e., leveraging the existing Option.EXTRA_OPEN_API_FORMAT_VALUES with the SimpleTypeModule could also be a way to do both at the same time.

configBuilder.with(Option.EXTRA_OPEN_API_FORMAT_VALUES, Option.ADDITIONAL_FIXED_TYPES);
// augment standard options, which are always applied last
configBuiilder.with(new SimpleTypeModule()
    // override default definition, that includes undesired "format" values
    .withIntegerType(Integer.class).withIntegerType(int.class)
    .withIntegerType(Long.class).withIntegerType(long.class)
    // add missing "format" values
    .withStringType(java.time.Duration.class, "duration")
    .withStringType(java.time.Period.class, "duration")
);

That being said, I'm open to discussing which types should get which "format" value by default and adding a new Option for that. What parts exactly would you prefer to be skipped and which are you missing?

@magicDGS
Copy link
Contributor Author

magicDGS commented Nov 7, 2023

Hello @CarstenWickner - thanks for the timely answer. When I was using the library on a playground to test it for my project, I realized that there is no way to include those "format" properties without having a custom module/format-resolver as you said (I have already used the Map-based snippet that you have provided). But I was thinking that it would be benefitial to include an Option to have those additional-types for the string as they are part of the built-in specs.

So to be clear, what I expect is the following (adding the JsonOrderProperty because I was using unit-testing for my playground and wanted to have an expected order to properly assert what I expected):

    @JsonPropertyOrder({
            "aInt", "aLong", "aFloat", "aDouble",
            "aLocalDate",
            "aLocalDateTime", "aZonedDateTime", "aOffsetDateTime", "aInstant", "aDate", "aCalendar",
            "aLocalTime", "aOffsetTime",
            "aDuration", "aPeriod",
            "aUuid", "aUri"
    })
    private static class ExpectedBuiltInFormats {
        ///////////////////
        // No built-in on the jsonschema specs
        // (but OpenAPI defined)
        //////////////////
        public Integer aInt; // expected: none (OpenAPI: int32)
        public Long aLong; // expected: none (OpenAPI: int64)
        public Float aFloat; // expected: none (OpenAPI: float)
        public Double aDouble; // expected: none (OpenAPI: double)

        ///////////////////
        // Dates and times
        ///////////////////
        public java.time.LocalDate aLocalDate; // expected: date
        public java.time.LocalDateTime aLocalDateTime; // expected: date-time
        public java.time.ZonedDateTime aZonedDateTime; // expected: date-time
        public java.time.OffsetDateTime aOffsetDateTime; // expected: date-time
        public java.time.Instant aInstant; // expected: date-time
        public java.util.Date aDate; // expected: date-time
        public java.util.Calendar aCalendar; // expected: date-time
        public java.time.LocalTime aLocalTime; // expected: time
        public java.time.OffsetTime aOffsetTime; // expected: time
        public Duration aDuration; // expected: duration
        public Period aPeriod; // expected: duration

        ///////////////////
        // Resource identifiers
        ///////////////////
        public java.util.UUID aUuid; // expected: uuid
        public java.net.URI aUri; // expected: uri
    }

What it was misleading and might be a good addition to the library is to handle the Option.ADDITIONAL_FIXED_TYPES (or something like an Option.BUILT_IN_JSONSCHEMA_FORMATS) to add those alredy implemented types without the need of the OpenAPI specific ones. So something like this test pass:

    @Test
    public void testWithAdditionalFixedTypes() {
        final SchemaGeneratorConfig config = new SchemaGeneratorConfigBuilder(SchemaVersion.DRAFT_2020_12, OptionPreset.PLAIN_JSON)
                .with(new JacksonModule(JacksonOption.RESPECT_JSONPROPERTY_ORDER))
                .with(Option.ADDITIONAL_FIXED_TYPES) // might be also posible to use a different option for this use-case
                .build();
       final ObjectNode node = new SchemaGenerator(config)
                .generateSchema(ExpectedBuiltInFormats.class);
        org.assertj.core.api.Assertions.assertThat(node.toPrettyString())
                .isEqualToIgnoringNewLines(EXPECTED_SCHEMA);
    }

Where the EXPECTED_SCHEMA is the following String:

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "type": "object",
  "properties": {
    "aInt": {
      "type": "integer"
    },
    "aLong": {
      "type": "integer"
    },
    "aFloat": {
      "type": "number"
    },
    "aDouble": {
      "type": "number"
    },
    "aLocalDate": {
      "type": "string",
      "format": "date"
    },
    "aLocalDateTime": {
      "type": "string",
      "format": "date-time"
    },
    "aZonedDateTime": {
      "type": "string",
      "format": "date-time"
    },
    "aOffsetDateTime": {
      "type": "string",
      "format": "date-time"
    },
    "aInstant": {
      "type": "string",
      "format": "date-time"
    },
    "aDate": {
      "type": "string",
      "format": "date-time"
    },
    "aCalendar": {
      "type": "string",
      "format": "date-time"
    },
    "aLocalTime": {
      "type": "string",
      "format": "time"
    },
    "aOffsetTime": {
      "type": "string",
      "format": "time"
    },
    "aDuration": {
      "type": "string",
      "format": "duration"
    },
    "aPeriod": {
      "type": "string",
      "format": "duration"
    },
    "aUuid": {
      "type": "string",
      "format": "uuid"
    },
    "aUri": {
      "type": "string",
      "format": "uri"
    }
  }
}

I think this won't be too difficult as they are already handled on the OpenAPI integration. If you accept contributions, I am also available to refactor to re-use the current definitions, add the duration ones, and add the option that you prefer.

@CarstenWickner
Copy link
Member

HI @magicDGS,

Your proposals sound good:

  • adding a new Option.STANDARD_FORMATS
  • refactoring the existing logic around the OpenAPI "format" values
  • extending the list of formats to cover the "duration" ones as well
  • fixing the @JsonPropertyOrder implementation, if it's not working as expected

I'd be happy to review and merge a PR for these changes (or whatever part you have time for).

@magicDGS
Copy link
Contributor Author

magicDGS commented Nov 8, 2023

Hello @CarstenWickner - the PR is ready: #399 - I also updated the documentation and added some tests.

The only think that I don't fully understand is what you mean with the @JsonPropertyOrder issue, as I was just adding it on my playground to ease my life when rendering the schema object and compared with my expectation.

@CarstenWickner
Copy link
Member

As per your separate question: this has been released as part of v4.33.0 now.

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