Current icarResource Discriminator property doesnt appear valid

[gra-moore]

The swagger seems incorrect: https://github.com/adewg/ICAR/blob/ADE-1/resources/icarResource.json with relation to the discriminator declaration.

According to the openAPI spec: https://swagger.io/specification/

While composition offers model extensibility, it does not imply a hierarchy between the models. To support polymorphism, the OpenAPI Specification adds the discriminator field. When used, the discriminator will be the name of the property that decides which schema definition validates the structure of the model. As such, the discriminator field MUST be a required field. There are two ways to define the value of a discriminator for an inheriting instance.

Use the schema name.
Override the schema name by overriding the property with a new value. If a new value exists, this takes precedence over the schema name. As such, inline schema definitions, which do not have a given id, cannot be used in polymorphism.

Key thing is that the descriminator property must be on the resource and must be required. From what I can see now this is not the case (unless it is a built in property of object).

The reason this is brought up is that resourceType is the name of the property we agreed on adding to support the streaming API. The PR to add this property will likely fix this issue. But we should discuss and see if this is really doing something we need and in the way we expect.

[AlexeyHardCode]

@gra-moore the original idea of adding discriminator to this resource was to be allowed other resources be inherited from icarResource.json. So I confirmed from Anton, that there was nothing behind this decision but this. So we can try to apply the fix. Have you already tried above proposed way to fix this issue?

[gra-moore]

@AlexeyHardCode Not quite sure I follow you? I understand the idea but as I said before I dont see how it would actually work given there is no resourceType property. My Proposal is to remove the discriminator, do you have an alternative?

[AlexeyHardCode]

@gra-moore I don't have any alternatives at the moment. I hope that removing the discriminator won't break the structure. I'll try to remove it locally and see if I can generate the code afterwards.

[AndreasSchultzGEA]

I tested different changes with our code-generator for Java and JS/TS:

• removing the discriminator leads to many problems, esp. in concerns of Generics, instanceof, etc. Large parts of our Java-code have to be re-written.
• defining the discriminator as a required property does not change anything at all for Java or JS/TS or the data-json.
• defining the discriminator as a dedicated property leads to replicated definitions in the data-json.
  example:
  "member": [ { "resourceType": "icarAnimalCoreResource", "resourceType": null, "@self": null,

Therefore, I think adding the required-definition is the solution which works best.

[gra-moore]

I have several questions, some technical some policy.

1. Policy question first, having a discriminator and not defining the property seems in violation of the swagger spec. So just because some of the tooling needs this, is this what we should be publishing?

2. When I read the spec it seems that to use the discriminator property you also need to specifiy a mapping between values of the discrinamator property values and Schemas.

"discriminator": {
"propertyName": "petType"
},
"properties": {
"name": {
"type": "string"
},
"petType": {
"type": "string"
}
},
"required": [
"name",
"petType"
]

When request bodies or response payloads may be one of a number of different schemas, a discriminator object can be used to aid in serialization, deserialization, and validation. The discriminator is a specific object in a schema which is used to inform the consumer of the specification of an alternative schema based on the value associated with it.

mapping | Map[string, string] | An object to hold mappings between payload values and schema names or references.

I don't see that we have defined these mappings anywhere, is this stuff that is generated by tooling?

3. We are about to add the resourceType property for our own usage, what happens when we fill it with values? Will this also break the tooling?

4. Out of interest what do serialised instances of the JSON look like that is produced with the tooling?

5. Not totally clear what your proposed solution looks like, can you post the schema as you see it could work, including that we are going to add resourceType as a property (but not a required one).

[AndreasSchultzGEA]

We are using the openapitools code-generator (v5.3.1) for Java, esp. Spring Boot.
I've been in close communication with Alexey, and he observed the same behaviour for the C#-Generator (csharp-dotnet code generator).

to 1.: No, we shouldn't publish sth. which is not conform to the requirements. That's true. Maybe there is a configuration-switch I didn't try to suppress the duplicated output within the data-json. Maybe not. But if there is not, we may solve this a different way.

to 2.: "I don't see that we have defined these mappings anywhere, is this stuff that is generated by tooling?" --> Yes, it is:
@ApiModel(description = "") @javax.annotation.Generated(value = "org.openapitools.codegen.languages.SpringCodegen", date = "2022-02-21T07:48:20.652605200+01:00[Europe/Berlin]") @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "eventType", visible = true) @JsonSubTypes({ @JsonSubTypes.Type(value = IcarAnimalSetJoinEventResource.class, name = "icarAnimalSetJoinEventResource"), @JsonSubTypes.Type(value = IcarAnimalSetLeaveEventResource.class, name = "icarAnimalSetLeaveEventResource"), @JsonSubTypes.Type(value = IcarConformationScoreEventResource.class, name = "icarConformationScoreEventResource"), @JsonSubTypes.Type(value = IcarDiagnosisEventResource.class, name = "icarDiagnosisEventResource"), @JsonSubTypes.Type(value = IcarFeedIntakeEventResource.class, name = "icarFeedIntakeEventResource"), @JsonSubTypes.Type(value = IcarLactationStatusObservedEventResource.class, name = "icarLactationStatusObservedEventResource"), @JsonSubTypes.Type(value = IcarMilkingDryOffEventResource.class, name = "icarMilkingDryOffEventResource"), @JsonSubTypes.Type(value = IcarMilkingVisitEventResource.class, name = "icarMilkingVisitEventResource"), @JsonSubTypes.Type(value = IcarMovementArrivalEventResource.class, name = "icarMovementArrivalEventResource"), @JsonSubTypes.Type(value = IcarMovementBirthEventResource.class, name = "icarMovementBirthEventResource"), @JsonSubTypes.Type(value = IcarMovementDeathEventResource.class, name = "icarMovementDeathEventResource"), @JsonSubTypes.Type(value = IcarMovementDepartureEventResource.class, name = "icarMovementDepartureEventResource"), @JsonSubTypes.Type(value = IcarReproAbortionEventResource.class, name = "icarReproAbortionEventResource"), @JsonSubTypes.Type(value = IcarReproDoNotBreedEventResource.class, name = "icarReproDoNotBreedEventResource"), @JsonSubTypes.Type(value = IcarReproHeatEventResource.class, name = "icarReproHeatEventResource"), @JsonSubTypes.Type(value = IcarReproInseminationEventResource.class, name = "icarReproInseminationEventResource"), @JsonSubTypes.Type(value = IcarReproMatingRecommendationResource.class, name = "icarReproMatingRecommendationResource"), @JsonSubTypes.Type(value = IcarReproParturitionEventResource.class, name = "icarReproParturitionEventResource"), @JsonSubTypes.Type(value = IcarReproPregnancyCheckEventResource.class, name = "icarReproPregnancyCheckEventResource"), @JsonSubTypes.Type(value = IcarReproStatusObservedEventResource.class, name = "icarReproStatusObservedEventResource"), @JsonSubTypes.Type(value = IcarTestDayResultEventResource.class, name = "icarTestDayResultEventResource"), @JsonSubTypes.Type(value = IcarTreatmentEventResource.class, name = "icarTreatmentEventResource"), @JsonSubTypes.Type(value = IcarTreatmentProgramEventResource.class, name = "icarTreatmentProgramEventResource"), @JsonSubTypes.Type(value = IcarWeightEventResource.class, name = "icarWeightEventResource"), }) public class IcarEventCoreResource extends IcarResource {
to 3.: The discriminator itself should not overlap with a semantical different property. Either we use a different property for the streaming API or for the resourceType-property used so far. As we all use generated code, there hopefully won't be any breaking change or code-refactorings. I would suggest to use a different property for the newer streaming-parts.

to 4:
{ "memberType": "icarReproPregnancyCheckEventCollection", "view": null, "member": [ { "eventType": "icarReproPregnancyCheckEventResource", "@self": null, "meta": { "source": "gea", "sourceId": "2f993df8-e4a6-4f28-af75-0d5a5c72a1a4", "modified": null, "created": null, "creator": null, "validFrom": null, "changeable": false, "validTo": null }, "id": "2f993df8-e4a6-4f28-af75-0d5a5c72a1a4", "animal": { "identifierType": "icarAnimalIdentifierType", "id": "4e271e6e-a68f-4c05-a8ab-110f398946d6", "scheme": "gea.cattleId" }, "eventDateTime": "2022-02-01T00:00:00+00:00", "location": { "identifierType": "icarLocationIdentifierType", "id": "9f886a13-c0c3-810c-1698-004ccaa7091e", "scheme": "gea.farmSiteToFarmMappingId" }, "traitLabel": null, "responsible": null, "contemporaryGroup": null, "method": null, "result": "Pregnant", "foetalAge": null, "foetusCount": null, "foetusCountMale": null, "foetusCountFemale": null, "exceptions": null } ] }

to 5: unclear what you are looking for

[cookeac]

Based on our discussion today, and some subsequent research:

• Discriminator is an OpenAPI specification feature intended to support inheritance in cases where it is unclear because anyOf or oneOf have been used, or in some cases when using allOf (which is what we use).
• OpenAPI specification states that a discriminator must reference a property will contain the name of the schema or an alternative value that is explicitly specified. This answers the question of what should be put in the discriminator.
• There is also a useful, short example of discriminator here: https://redoc.ly/docs/resources/discriminator/

This would all have been fine but:

• The open api generator appears to have a design decision some people think is a bug where if a discriminator is declared and the property is also declared (i.e. valid OpenAPI spec), it will generate two declarations of that property in generated code, which is what Andreas and Alexey have seen. Maybe Jackson Annotation @JSONTypeInfo was designed for serialising Java Objects, not specifically for implementing code generation from OpenAPI, and the merger of the two purposes has confused the intent. It appears this MAY be fixed in some implementations.
• As a result of the above, when discriminator was first implemented in the ICAR schemas in 2019/2020, the property was not declared.
• The end result is that any implementations that don't use the open-api-generator or a compatible generator (for instance, hand-coded implementations, or those using an XML/JSON mapping tool, etc) may not add and fill in the property, so their output will not be able to be read by those using open-api-generator. This is a problem. Of course, if they have tested against an open-api-generator version, they may have found and worked around the problem themselves.

Potential approach

• If all current implementers of the specification were using a open api generator or something compatible, then we could effectively assume that the discriminator properties have effectively been "required", and we could properly define them and make them required without breaking existing implementations (but only if the Jackson issue above was fixed or worked around).
• However, if people have taken a different route (for instance, hand-implemented classes to comply with the spec or used a different generator) they won't have had these properties and won't have known to fill them with the name of the concrete class, so this would be a breaking change for them.
• Alternatively, we could use but document the current situation, which is that the specification does not fully comply with OpenAPI but does comply with most common API generators. We would need to explain how a property should be implemented for each discriminator, and populated with the schema name. Again, those who are hand-coding implementations would find this (documentation) a breaking change.
• If either of these was done, and well documented, we could use that same semantic for purposes such as the streaming API, which is using resourceType to understand the type of resource when deserialising it from an API..

Note that we have used different names for these discriminators through the ICAR specification: icarIdentifierType.identifierType, icarResource.resourceType, icarEventCoreResource.eventType, and icarResourceCollection.memberType. The confusion between eventType and resourceType concerns me. It looks to me like the code generators take the outermost discriminator as the discriminator they use (eventType). An implementer wanting to implement the serialisation API in this case would need to ensure they populated both resourceType AND eventType, which is not ideal.

[gra-moore]

Agree with @cookeac summary above and highlight:

1. The value in the discriminator needs to be the name of the schema (type) in the schema document, or a shortname that is used in the mapping. For consistent interchange ideally, implementations would use a mapping that referenced a normative published OpenAPI spec document. But as Andrew says at least we know that these values arent java class names.

2. That the discriminator must reference a declared property. This is super important as it makes it clear that a client must supply this property and it is in the Open API spec.

3. I also worry about the inconsistency and hierarchy of the discriminators. EventType extends Resource yet defines is own discriminator - this must be wrong. I would imagine that each distinct type hierarchy can choose the root level discriminator property.

4. For the streaming API if the property was required and values were the names of types then for this version of the API we can just treat them as short names for the semantic types.

My proposal for minimal disruption and max compliance:

1. Each resource type root defines the discriminator (e.g. not EventType)
2. The resourceType Property is declared and made required.
3. We document that the properties must be Schema names. (this is kind of implied anyway)
4. Streaming API uses these for type differentiation (e.g. we don't permit the URIs in there)

Impact:

• find flag or fix in generators to deal with this according to the spec
• regen as the names of discriminators have changed
• Given no one has flagged this before we can be fairly safe in saying there has been no service to service calls between different implementors - so all changes can be dealt with by each actor.
• As this property is only about aid to serialisation no business logic should be affected.

From an opinion perspective - a goal of making it simpler to use this spec by supporting autogeneration tooling should not lead to compromising the long term correctness due to bugs in that tooling.

[gra-moore]

I just tested out the Open API go generator. It produces the following structure based on the current ADE-1 version of the spec:

type IcarResource struct { // Uniform resource identifier (URI) of the resource (rel=self). Self *string json:"@self,omitempty"Meta *IcarMetaDataTypejson:"meta,omitempty" }

with event subtype represented as:

// IcarEventCoreResource type IcarEventCoreResource struct { IcarResource // Unique identifier in the source system for this event. Id *string json:"id,omitempty"Animal *IcarAnimalIdentifierTypejson:"animal,omitempty"// udt:DateTimeType | A particular point in the progression of time together with relevant supplementary information. EventDateTime *time.Timejson:"eventDateTime,omitempty"Location *IcarLocationIdentifierTypejson:"location,omitempty"TraitLabel *IcarTraitLabelIdentifierTypejson:"traitLabel,omitempty"// Use if an observation is manually recorded, or an event is carried out or authorised by a person. SHOULD be a person object. Responsible *stringjson:"responsible,omitempty"// For manually recorded events, record any contemporary group code that would affect statistical analysis. ContemporaryGroup *stringjson:"contemporaryGroup,omitempty" }

The serialisation is done such:

func (o IcarEventCoreResource) MarshalJSON() ([]byte, error) { toSerialize := map[string]interface{}{} serializedIcarResource, errIcarResource := json.Marshal(o.IcarResource) if errIcarResource != nil { return []byte{}, errIcarResource } errIcarResource = json.Unmarshal([]byte(serializedIcarResource), &toSerialize) if errIcarResource != nil { return []byte{}, errIcarResource } if o.Id != nil { toSerialize["id"] = o.Id } if o.Animal != nil { toSerialize["animal"] = o.Animal } if o.EventDateTime != nil { toSerialize["eventDateTime"] = o.EventDateTime } if o.Location != nil { toSerialize["location"] = o.Location } if o.TraitLabel != nil { toSerialize["traitLabel"] = o.TraitLabel } if o.Responsible != nil { toSerialize["responsible"] = o.Responsible } if o.ContemporaryGroup != nil { toSerialize["contemporaryGroup"] = o.ContemporaryGroup } return json.Marshal(toSerialize) }

Neither the resourceType nor eventType property are added to the serialised representations.

I conclude from this that any clients generating code for use in Go will not work when talking to services for other languages.

This is the generator command to run to see the above.

docker run --rm -v "${PWD}:/local" openapitools/openapi-generator-cli generate \ -i https://raw.githubusercontent.com/adewg/ICAR/ADE-1/url-schemes/milkURLScheme.json \ -g go \ -o /local/out/go

[gra-moore]

It also appears that the kotlin generator does not suffer from the same bug as the java generator as it uses a different JSON serialiser. Again, the property is not found on the model constructs. Any kotlin client will not work with any services that require the resourceType field to be set.

Here is the kotlin data structure for event

`**
*
*

• @param atSelf Uniform resource identifier (URI) of the resource (rel=self).
• @param meta
• @param id Unique identifier in the source system for this event.
• @param animal
• @param eventDateTime udt:DateTimeType | A particular point in the progression of time together with relevant supplementary information.
• @param location
• @param traitLabel
• @param responsible Use if an observation is manually recorded, or an event is carried out or authorised by a person. SHOULD be a person object.
• @param contemporaryGroup For manually recorded events, record any contemporary group code that would affect statistical analysis.
  */

interface IcarEventCoreResource : IcarResource {

/* Unique identifier in the source system for this event. */
@Json(name = "id")
val id: kotlin.String?
@Json(name = "animal")
val animal: IcarAnimalIdentifierType?
/* udt:DateTimeType | A particular point in the progression of time together with relevant supplementary information. */
@Json(name = "eventDateTime")
val eventDateTime: java.time.OffsetDateTime?
@Json(name = "location")
val location: IcarLocationIdentifierType?
@Json(name = "traitLabel")
val traitLabel: IcarTraitLabelIdentifierType?
/* Use if an observation is manually recorded, or an event is carried out or authorised by a person. SHOULD be a person object. */
@Json(name = "responsible")
val responsible: kotlin.String?
/* For manually recorded events, record any contemporary group code that would affect statistical analysis. */
@Json(name = "contemporaryGroup")
val contemporaryGroup: kotlin.String?

}`

[gra-moore]

One final note. The dotnet generator recommended by Microsoft is here:

https://docs.microsoft.com/en-us/aspnet/core/tutorials/getting-started-with-nswag?view=aspnetcore-6.0&tabs=netcore-cli

NSwag uses this library for its JSON Schema: And here is the page that describes how discriminators work:

https://github.com/RicoSuter/NJsonSchema/wiki/Inheritance

From what I can see this dotnet tooling library is compliant with the specification that a discriminator must be specified as a property.

I have not had a chance to run this tool on the open api files.

[cookeac]

There are several issues logged in openapi-generator about this. Essentially it seems that at present discriminators to support data binding and model properties are handled by two different parts of the generator code, and the "discriminator" part does not align with the OpenAPI spec (i.e. doesn't expect the discriminator will be a property in the model).

It is possible to use annotations (As.EXISTING_PROPERTY) to tell the Java and C# generator to use the model property, but then it expects you to populate the value of the property yourself (which is how other languages will do it anyway).

It does look like this is being addressed, filling in the model property automatically on serialization, which has recently been merged into master (6.0.0) for openapi-generator. See #9441 in https://github.com/OpenAPITools/openapi-generator/issues. I have no idea what that means though in actual usable releases in the roadmap.

[cookeac]

We agreed that it makes sense to try and align our ADE 1.3 release with when the repaired openapi-generator (6.0.0) is available. However, it could be released earlier if that is delayed.

At the time we release 1.3, as there are not any semantic changes people can either:

• Adopt 1.3 with updated tooling if required
• Adopt 1.3 and manually adjust their code generation
• Remain with 1.2

We also agreed to tidy away the unnecessary discriminators in classes derived from ResourceType (collections and events).
Finally, the discriminator for IdentifierTypes is not really needed, but we should ensure this does not stop code generating.

[cookeac]

Discussed 21/04/2022:
Now that the PRs for this have been approved, we need to test code generation under Java and C# with the openapi-generator 6.0.0 beta to see whether updated tooling handles the corrected resourceType discriminator correctly.
@AndreasSchultzGEA volunteered to try this for Java, and we hope @AlexeyHardCode might have time to look at this for C#.

[cookeac]

Fixed by #279
Fixed by #287

[cookeac]

Reactivated this issue so we could discuss in the context of #295 and #299
We decided that @AndreasSchultzGEA will test again with the code generator (last tested in May) to see if any newer releases to openapi-generator beta 6 have improved things.