RepreZen / KaiZen-OpenApi-Parser

High-performance Parser, Validator, and Java Object Model for OpenAPI 3.x
130 stars 31 forks source link

JsonOverlay should not remove security: [] #235

Open chris-brace opened 5 years ago

chris-brace commented 5 years ago

I am serializing a OAS3 model to YAML using the following boilerplate described in the docs

final JsonNode serial = Overlay.of(model).toJson(SerializationOptions.Option.FOLLOW_REFS);
final String raw = new YAMLMapper().writerWithDefaultPrettyPrinter().writeValueAsString(serial);

My API has a default security scheme applied to all routes except one, which defines security: [] to override the root security object as is (extremely poorly) described in "step two" of this document https://swagger.io/docs/specification/authentication/ . The serializers remove this, so right now i am doing some gross string substitution to fix it.

The point is, that empty list has significance.

Also, I am aware that I can disable removal of all empty lists and objects using serialization options, but this is not what i want to do as the output generates tons of errors and warnings from parsers and editors.


In case anyone else ends up here my gross solution is, for the following spec:

security:
  - JWTAuth: []
paths:
  /:
    get:
      security:
        - JWTAuth: ["DISABLE"]

I do the following to the final, serialized, prettyprinted YAML file:

private static String fixupYAML(final String in)
{
    // This is what happens when you do - Foo: []
    return in.replaceAll("JWTAuth: null", "JWTAuth: []")
             // Because the serializer removes all empty objects and arrays
             .replaceAll("security:\n {6}- JWTAuth:\n {8}- \"DISABLE\"", "security: []");
}
tedepstein commented 5 years ago

@chris-brace , not sure whether it's helpful to point this out, but it seems what you want is an empty array, not an empty object. Is there a serialization option to remove empty objects, but preserve empty arrays? If so, would that still give you too much noise in the serialized OpenAPI doc, and ensuing errors, warnings, etc.?

Is there a way to explicitly set the value of the security property to empty array, and would that change how it serializes?

Sorry that I don't know the answers to these questions, and I'm not set up to look at it right now. But if my musings here don't lead to a solution, I will take a closer look, hopefully next week.

tedepstein commented 5 years ago

A hunch: SecurityRequirementImpl seems to need a fix similar to the one we see in SecurityParameterImpl.

Security Requirement Object is a map of Security Scheme names to an array of scope requirements for that security scheme. In KZOP, SecurityParameter represents the string scopes array, i.e. the map value in the Security Requirement Object. It's defined here in types3.yaml, the config file for the JSON Overlay generator.

- name: SecurityParameter
  imports:
    impl:
    - JsonNodeFactory
  fields:
    parameter:
      type: String
      parentPath: ''
      structure: collection

I think the JsonNodeFactory import is needed for this code, which I think is what provides the empty array if the property value is missing.

    @Override
    protected JsonNode _fixJson(JsonNode json) {
        return json.isMissingNode() ? _jsonArray() : json;
    }

At first glance, it seems like we need a similar solution for SecurityRequirements.

We know this works for SecurityParameter, because OpenAPI requires empty scope arrays on Security Requirement Objects for schemes of any type other than oauth2 or openIdConnect. The requirement for empty scope array is explicit in the spec, and is very commonly used in lots of API documents.

Empty security requirements, allowing anonymous access, are not as common. It's not even clear (enough) in the OpenAPI spec that it's permitted. See the open PR #1886 here and the referenced issues.

It seems that the proposed syntax for this is security: [ {} ], not security: [].

Empty security requirements array as a security value is less common. But the spec explicitly allows this as an operation-level override of a top-level security requirement:

To remove a top-level security declaration, an empty array can be used.

The open pull request #1886 adds explicit support for an empty Security Requirement Object in the array. It's intended for cases where security is optional, as in this example, showing the empty object in an array with other, non-empty Security Requirement Objects.

I'm guessing you'll hit the same problem, trying to create an array with a single, empty Security Requirement object.

This needs a little more thought...

chris-brace commented 5 years ago

Sorry, I edited my issue to be specific that what i want is, in the case that it is present, an empty list for security to persist through serialization because it is meaningful.

tedepstein commented 5 years ago

@chris-brace , just to cover the bases, it would help to know:

tedepstein commented 5 years ago

@chris-brace , I took some time this weekend to experiment with serialization, focusing on several cases of empty objects and empty arrays that are meaningful in Security Scheme and Security Requirement objects. You can see an OpenAPI YAML test input and failing JUnit test in the task/#235 branch.

It seems that the problems with serialization are pretty extensive. There is the big problem of empty objects and empty arrays described in the docs:

N.B. Neither treatment distinguishes between empty structures that were created automatically during the elaboration process, and empty structures that were present in the source model (or added via the mutation API). This is especially problematic in certain cases where empty structures actually mean something different from missing structures in the OpenAPI specification. For example, an empty servers list in an Operation Object overrides servers objects at the path or root level in the model, but a missing servers object does not. Until this issue is addressed properly, toJson() can yield models that differ semantically from their intended meaning. Round-tripping through the parser and the serializer can change the meaning of a model.

Also described in issue #114.

I am also seeing that flows are completely missing from the serialized JSON, which may or may not be explained by the known issue with empty objects. It looks like it might be a separate bug, but I haven't really tried to analyze it yet.

So it seems that serialization is incomplete, not ready for real use at this point. You can use the parser to read, traverse, and inspect an OpenAPI model (which is what our application code does), even modify the OpenAPI model in-memory, but not to write changes back to a file.

The fix for these problems is going to require some design changes that are beyond what we can do right now. I'd like to keep this open as a back-burner issue, and try to rework the design to handle serialization properly. But I can't offer a time frame for this.

chris-brace commented 5 years ago

Thank you for your thorough analysis! I understand what’s wrong and perhaps at some point I’ll be able to take a look for myself. In the mean time I’ve gotten it working with the little hacks I described :)