swagger-api / swagger-core

Examples and server integrations for generating the Swagger API Specification, which enables easy access to your REST API
http://swagger.io
Apache License 2.0
7.37k stars 2.17k forks source link

Wrong documentation when nullable property is used in a field that maps to object #3518

Closed elgleidson closed 2 weeks ago

elgleidson commented 4 years ago

First of all, the following repository has all the code (and description as well) to reproduce this problem: https://github.com/elgleidson/swagger-problem

I have the following JSON:

{
  "nonNullableField": "not null",
  "nullableField": null,
  "nonNullableObjectField": {
    "someField": "some value"
  },
  "nullableObjectField": null,
  "nonNullableList": [
    "not null"
  ],
  "nullableList": null,
  "nonNullableObjectList": [
    {
      "someField": "some value"
    }
  ],
  "nullableObjectList": null
}

Which is mapped to the following Java classes:

@Value
public class MyResponse {

  @Schema(nullable = false, description = "DO NOT map to json object and DO NOT allow null")
  private final String nonNullableField = "not null";

  @Schema(nullable = true, description = "DO NOT map to json object and allows null")
  private final String nullableField = null;

  @Schema(nullable = false, description = "map to json object and DOES NOT allow null")
  private final MyClass nonNullableObjectField = new MyClass(nonNullableField);

  @Schema(nullable = true, description = "map to json object and allows null")
  private final MyClass nullableObjectField = null;

  @ArraySchema(arraySchema = @Schema(nullable = false, description = "list that DOES NOT map to json object and DOES NOT allow null"))
  private final List<String> nonNullableList = List.of(nonNullableField);

  @ArraySchema(arraySchema = @Schema(nullable = true, description = "list that DOES NOT map to json object and allow null"))
  private final List<String> nullableList = null;

  @ArraySchema(arraySchema = @Schema(nullable = false, description = "list that map to json object and DOES NOT allow null"))
  private final List<MyClass> nonNullableObjectList = List.of(nonNullableObjectField);

  @ArraySchema(arraySchema = @Schema(nullable = true, description = "list that map to json object and allow null"))
  private final List<MyClass> nullableObjectList = null;

}

@Value
@Schema(description = "my class description")
public class MyClass {

  @Schema(description = "my class field description")
  private final String someField;

}

When I go to /v3/api-docs (or /swagger-ui.html) the following documentation is generated:

{
  "MainResponse": {
    "type": "object",
    "properties": {
      "nonNullableField": {
        "type": "string",
        "description": "DO NOT map to json object and DO NOT allow null"
      },
      "nullableField": {
        "type": "string",
        "description": "DO NOT map to json object and allows null",
        "nullable": true
      },
      "nonNullableObjectField": {
        "$ref": "#/components/schemas/MyClass"
      },
      "nullableObjectField": {
        "$ref": "#/components/schemas/MyClass"
      },
      "nonNullableList": {
        "type": "array",
        "description": "list that DOES NOT map to json object and DOES NOT allow null",
        "items": {
          "type": "string"
        } 
      },
      "nullableList": {
        "type": "array",
        "description": "list that DOES NOT map to json object and allow null",
        "nullable": true,
        "items": {
          "type": "string"
        }
      },
      "nonNullableObjectList": {
        "type": "array",
        "description": "list that map to json object and DOES NOT allow null",
        "items": {
          "$ref": "#/components/schemas/MyClass"
        }
      },
      "nullableObjectList": {
        "type": "array",
        "description": "list that map to json object and allow null",
        "nullable": true,
        "items": {
          "$ref": "#/components/schemas/MyClass"
        }
      }
    }
  },
  "MyClass": {
    "type": "object",
    "properties": {
      "someField": {
        "type": "string",
        "description": "my class field description"
      }
    },
    "description": "my class description",
    "nullable": true
  }
}

As you can see, for the fields whose types are not mapped to object the documentation is generated as expected. The same doesn't happen for nullableObjectField: the nullable property is put in MyClass definition instead of the field.

The following documentation should be generated instead:

{
  "MainResponse": {
    "type": "object",
    "properties": {
      "nonNullableField": {
        "type": "string",
        "description": "DO NOT map to json object and DO NOT allow null"
      },
      "nullableField": {
        "type": "string",
        "description": "DO NOT map to json object and allows null",
        "nullable": true
      },
      "nonNullableObjectField": {
        "$ref": "#/components/schemas/MyClass",
        "description": "map to json object and DOES NOT allow null"
      },
      "nullableObjectField": {
        "$ref": "#/components/schemas/MyClass",
        "description": "map to json object and allows null",
        "nullable": true
      },
      "nonNullableList": {
        "type": "array",
        "description": "list that DOES NOT map to json object and DOES NOT allow null",
        "items": {
          "type": "string"
        } 
      },
      "nullableList": {
        "type": "array",
        "description": "list that DOES NOT map to json object and allow null",
        "nullable": true,
        "items": {
          "type": "string"
        }
      },
      "nonNullableObjectList": {
        "type": "array",
        "description": "list that map to json object and DOES NOT allow null",
        "items": {
          "$ref": "#/components/schemas/MyClass"
        }
      },
      "nullableObjectList": {
        "type": "array",
        "description": "list that map to json object and allow null",
        "nullable": true,
        "items": {
          "$ref": "#/components/schemas/MyClass"
        }
      }      
    }
  },
  "MyClass": {
    "type": "object",
    "properties": {
      "someField": {
        "type": "string",
        "description": "my class field description"
      }
    },
    "description": "my class description"
  }
}
webron commented 4 years ago

This is going to be challenging to investigate. Looking at the linked issue, it looks like you're using springdoc, and they sent you here because they claim to be using our processor (which they might). However, officially, we don't support Spring MVC/Boot as they use a different annotatiions to describe operations.

We're very grateful for the sample project, however, in order to isolate that the issue really is in swagger-core and not springdoc, we'd need a sample that's based on JAX-RS and not spring boot to reproduce it. Is there any chance you can produce such sample?

bnasslahsen commented 4 years ago

Hi @webron,

Here's a sample code (pure java) that helped us confirm the reproduce. It's using MyResponse class mentioned by @elgleidson.

I wish it could help.

ResolvedSchema resolvedSchema = ModelConverters.getInstance()
        .resolveAsResolvedSchema(new AnnotatedType(MyResponse.class));
if (resolvedSchema.schema != null) {
    Schema schemaN = resolvedSchema.schema;
    Map<String, Schema> schemaMap = resolvedSchema.referencedSchemas;
    StringSchema stringSchema = (StringSchema) schemaMap.get("MyResponse").getProperties().get("nonNullableField");
    if (stringSchema.getNullable() == null) {
        throw new IllegalArgumentException("nonNullableField, should not be null");
    }
}
frankruegamer commented 3 years ago

Is there any progress on this issue?

waage commented 3 years ago

Any progress?

davidmelia commented 1 year ago

any progress?

maxLBCarbon commented 1 year ago

Hello :)

Any progress on this bug ?

const commented 1 year ago

First thing, the expected definition like:

      "nonNullableObjectField": {
        "$ref": "#/components/schemas/MyClass",
        "description": "map to json object and allows null",
        "nullable" : true   
      }

is incorrect. Becase "$ref" replaces definition, and all sibling properties it will be ignored. See https://swagger.io/docs/specification/using-ref/ .

$ref and Sibling Elements

Any sibling elements of a $ref are ignored. This is because $ref works by replacing itself and everything on its level with the definition it is pointing at.

So, event if this is forced, the importing tools will likely ignore it.

I think expected variant is:

      "nullableObjectField": {
        "oneOf" : [{"$ref": "#/components/schemas/MyClass"}],
        "description": "map to json object and DOES NOT allow null",
        "nullable" : true   
      }

However it is not easy to implement becase the ref still wanted even if oneOf specified in swagger.

I have put extension on the container object:

  @Schema(
      description = "My dto",
      name = "MyDto,
      extensions =
          @Extension(
              name = "x-force-null",
              properties =
                  @ExtensionProperty(name = "myNullableProperty", value = "true")))
  class MyDto {
    @Schema(description = "something or null")
    OtherDto myNullableProperty;
  }

Note, extensions are ignored on ref properties as well. Then I wrote OpenApiCustomiser customizer in Spring that walks over schema and does the following:

 @SuppressWarnings("unchecked")
  public void processSchema(Schema<?> schemaModel) {
    if (schemaModel.getExtensions() != null) {
      var map = (Map<String, Object>) schemaModel.getExtensions().get("x-force-null");
      if (map != null) {
        if (schemaModel.getProperties() != null) {
          for (String property : map.keySet()) {
            var type = schemaModel.getProperties().get(property);
            if (type.get$ref() != null) {
              var t = new ComposedSchema();
              t.setNullable(true);
              t.setOneOf(List.of(type));
              schemaModel.getProperties().put(property, t);
            } else {
              type.setNullable(true);
            }
          }
        }
        if (schemaModel.getExtensions().size() == 1) {
          schemaModel.setExtensions(null);
        } else {
          schemaModel.getExtensions().remove("x-force-null");
        }
      }
    }
  }

Basically, it extension presents, it converts all ref properties mentioned in it to oneOf and then remove extension from schema.

This works for me, because with have a single consumer of the result schema that understands this variant. Result could differ for other tools.

I think the correct implementation would be if anything property-specific like description, nullable, extensions are specifed for reference type, then oneOf type should be automatically used with a single candidate, and other things should be put near oneOf. Note that even for nonNullableField the description is lost, and this is an information loss because we now do not know what is local semantics of the field as $ref specifies global semantics and syntax.

elgleidson commented 5 months ago

@const sorry for the late reply.

I didn't test your suggestion, but I did some things in my example to work around the issue: using Jakarta (previously Javax) annotations:

@Value
public class MyResponse {

  @NotBlank
  @Schema(nullable = false, description = "DOES NOT map to json object and DOES NOT allow null")
  private final String nonNullableField;

  @Schema(nullable = true, description = "DOES NOT map to json object and DOES allow null")
  private final String nullableField;

  @Schema(nullable = false, description = "DOES map to json object and DOES NOT allow null")
  @NotNull
  private final MyClass nonNullableObjectField;

  @Schema(nullable = true, description = "DOES map to json object and DOES allow null")
  private final MyClass nullableObjectField;

  @ArraySchema(arraySchema = @Schema(nullable = false, description = "list that DOES NOT map to json object and DOES NOT allow null"))
  @NotEmpty
  private final List<String> nonNullableList;

  @ArraySchema(arraySchema = @Schema(nullable = true, description = "list that DOES NOT map to json object and DOES allow null"))
  private final List<String> nullableList;

  @ArraySchema(arraySchema = @Schema(nullable = false, description = "list that DOES map to json object and DOES NOT allow null"))
  @NotEmpty
  private final List<MyClass> nonNullableObjectList;

  @ArraySchema(arraySchema = @Schema(nullable = true, description = "list that DOES map to json object and DOES allow null"))
  private final List<MyClass> nullableObjectList;

}

Which results in the following specification (just the important bit):

{
  "MyResponse": {
    "required": [
      "nonNullableField",
      "nonNullableList",
      "nonNullableObjectField",
      "nonNullableObjectList"
    ],
    "type": "object",
    "properties": {
      ...
    }
  }
}

Visually, I get this (red * in the required fields):

image
elgleidson commented 5 months ago

In regards to the $ref definition, I see it as a contradiction between its definition:

When you document an API, it is common to have some features which you use across several of API resources. In that case, you can create a snippet for such elements in order to use them multiple times when you need it. With OpenAPI 3.0, you can reference a definition hosted on any location. It can be the same server, or another one – for example, GitHub, SwaggerHub, and so on. To reference a definition, use the $ref keyword:

And its implementation:

Any sibling elements of a $ref are ignored. This is because $ref works by replacing itself and everything on its level with the definition it is pointing at. Consider this example:

components:
schemas:
Date:
type: string
format: date
DateWithExample:
$ref: '#/components/schemas/Date'
description: Date schema extended with a `default` value... Or not?
default: 2000-01-01

In the second schema, the description and default properties are ignored, so this schema ends up exactly the same as the referenced Date schema.

Their example makes things even weird: a Date could be used as "date of birth", not nullable, no defaults, in one place, but being used as a "end date", nullable, in other place.

How nobody thought that a component that's suppose to be reused in many places could:

I hope they can review this design/implementation decision in a 3.2.x API definition.

Anyway, I appreciated the time you spent replying to my issue.

Burtsev-Alexey commented 2 months ago

Same Issue here. We have this class setup:

@Schema(title = "Payment") public record PaymentData( @Schema(title = "Amount paid from wallet") MoneyAmount paidWallet, @Schema(title = "Amount paid from card") MoneyAmount paidCard) { } In swagger-ui we see as description for paidWallet and paidCard nor "Amount paid from wallet" nor "Amount paid from card", its some random? text from other usages of MoneyAmount in project....

micryc commented 2 weeks ago

@elgleidson @Burtsev-Alexey Hi, sorry for pinging but, out of curiosity, have you tried to generate 3.1 openapi specification ? Does the problem still appear ?

micryc commented 2 weeks ago

I am closing this ticket for now, feel free to reopen if there will be still any issues even with 3.1 generated spec

Burtsev-Alexey commented 1 week ago

I'm not sure how to check 3.1 version. We are using Swagger from Spring Boot 3 app and it have this dependencies image

micryc commented 1 week ago

@Burtsev-Alexey To generate openapi spec 3.1 version, you have to add a custom springdoc property, in your spring-boot configuration file. Screenshot 2024-09-11 at 08 12 29

Burtsev-Alexey commented 1 week ago

@micryc Thank you, i have switched to 3.1 and now schema is generated properly I see: "paidWallet": { "$ref": "#/components/schemas/MoneyAmount", "title": "Amount paid from wallet" }, "paidCard": { "$ref": "#/components/schemas/MoneyAmount", "title": "Amount paid from card" } It was incorrect before 3_1: "paidWallet": { "$ref": "#/components/schemas/MoneyAmount" }, "paidCard": { "$ref": "#/components/schemas/MoneyAmount" }

The minor problem is titles are not displayed with 3_1 in swagger-ui, but this is other problem I will probably figure this out some day...

So I think this issue is resolved.