Problem reading vendor extensions applied of properties defined by an external reference

[mwilby]

I noticed this problem a while ago. I did report it to the penapi-generator/issues/14327 . At the time it wasn't a big issue and I suspected it was a problem with the swagger parser. Recently, it has become something of a more important issue for us, so I looked into how to fix it. Fixing it is not too difficult, what's difficult is deciding on if it should be fixed, or not.

All the following will refer to version 2.1.14 when mentioning line numbers. I figure this is fixed, where the current development branch is not.

I will explain the problem in the context of the parser. Starting with the schema definition, where we deserialize this simple model description

test_message:
   type: object
   properties:
      primitive:
         type: integer
         x-format: u32
         x-maitred-index: 0
      in_line:
         type: object
         x-maitred-index: 1
         properties:
            id:
               description: Vehical registation ID.
               type: string
               x-maitred-index: 0
         description: in_line description
      ref_obj:
         $ref: '#/ref_obj'
         x-maitred-index: 2
         description: ref_obj external description

ref_obj:
   type: object
   required:
      - id
   description: ref_obj internal description
   properties:
      id:
         description: Vehical registation ID.
         type: string
         x-maitred-index: 0

When you are deserializing the parameters and you call io.swagger.v3.parser.util.OpenAPIDserializer.getSchema() an the node generated for the last parameter ref_obj, you send in the following JsonNode

{
    "description":"ref_obj external description",
    "x-maitred-index":2,
    "$ref":"net_path.yaml#/net_path"
}

The getSchema() method recognizes this as a ref and on line 2760 enters an if statement to load the referenced object. All that's fine, but once that's done, on line 2799 the loaded schema is returned. This means that the description is ignored, which it probably should be, but also the vendor extension field is ignored. This I would argue is a a error. The vendor extension on this parameter are important, at least to us.

Among the other fields that are also ignored are:

• default
• nullable

These might also be important.

A quick hack fix is easy. Just replace line 2799 with

Map<String, Object> extensions = getExtensions(node);
if (extensions != null && extensions.size() > 0) {
    schema.setExtensions(extensions);
}
return schema;

I am sure there is a much better solution, but this would fix our problem.

The big issue is that this change would have an effect on openapi-generator, and probably all other generators. Specifically, it would add to the vendor generated fields. In the case of openapi-generator it will only append extra fields, so in all probability no one would notice it, but there is no guarantee that other generators would not be effected more drastically.

These parameter fields on reference are a complex issue. Some of them can be ignored, but some of them have meanings that make sense, so I think they should be included.

So, is this a change that can be made, or not?

[drieks]

Maybe it's possible that schema also has extensions, using schema.setExtensions(extensions); should not overwrite them.

[mwilby]

What I am proposing does not effect the assignment relative to the original call to setExtensions when loading the referenced object. It refers to the interaction between the values assigned to a property and the values the object got from its original assignment from setExtensions.

It is true it is possible this problem exists in some systems that use the parser, hence why I posed this as a question. But to clarify my original post, this issue does not apply to the openapigen generator. The issue is, are other generators effected? Also, as I point out below the issue is not so simple, so I think you will have to do something about how this operation works to clarify the specification regardless.

Let me explain why I don't think it effects openapigen.

What the parser does is build a map with a reference value to represent this entry. Specifically, the property value contains a reference to the object, not a copy of it. At the moment it does not add the property external definitions, so you just have the reference. Then this generated structures is used by the openapigen generator.

The generator processes this structures into a JSON like map and then performs the following steps:

1. it loops over the property fields of an object, loading the values of the property, in the case of interest this is only the reference link.

2. Then, in this case, it recognizes it as a referenced object.

3. It then goes and loads the referenced object, including the values set by setExtensions and in the case where the external values of the referenced object exist it does the following:

   • if the original property object does have a null external object (true because it was not loaded), it creates an empty map.
   • now it loops over the external value of the ref object and uses the Java Map.put() method to insert these external value into the local external map.

   The put method overwrites existing values (it does return the replaced value, but its not used here).

   The result is that the external values of referenced object would overwrite any added from the property values, so from the users point of view all they would see is more entries added to what existed before, but no change to the original values. So its not going to effect openapigen. I think this is what should happen, but also with a warning telling the user that the value has been overwritten.

   HOWEVER, this only happens if the implementation follows this pattern, if there is a generator out there that depends on the parser, that implements this interaction in the reverse order, it would see the effect you mentioned. Possible even worse, Its behavior would be undefined, if the implementation had been more carefully built and the overwritten values are checked and processes. But having implementation dependent behavior is a set of problems that will blow up in the future regardless, so its worth looking into it anyway.

   The problem is not really a bug, or an implementation issue, its a design specification issue. I originally thought that the best thing to do would be to add an entry in the external map the used a key of something like"REF_VALUES", or anything that does not start withx-, and then make the value be the external value map of the referenced object. However this gets into issues about the protocol specification, potential problems with validation functions and the generators would need to learn to use the new feature, so its a none starter of a solution. I only mention this to demonstrate its a specification problem.

   In addition, its not necessary to change the parser to do this, because the objects the parser generates already have the implicate reference definition built in, its just missing the other data. The reason I think you will have to do something about it, is that, as happens a lot, there is a difference in the behavior depending on if the object is a reference, or if it is defined "in line". If you look at my original posting, openapi-generator/issues/14327, you can see it works correctly (from my perspective) for the in line case, and not for the referenced case. It should at least be consistent.

   In addition as I pointed out there are other fields that are also ignored. Specifically, "default" and "nullable". I think default will only be a problem for externally defined Enums, but "nullable" is an issue for general objects as well.

   As I said I think the problem here is in the specification definition, and there is a problem if you change things, and one if you don't. The formal behavior of the specification, as it stands is ambiguous and that has to be changed somehow.

   For my part I think the current behavior is just wrong from a logical perspective.

[rimokhy]

Hello, thanks @mwilby for the bug report. I've been encountering the same issue, any update or workaround on this matter ?