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

Release notes required to explain 1.3 changes #358

Closed
cookeac opened this issue Dec 15, 2022 · 4 comments · Fixed by #367
Closed

Release notes required to explain 1.3 changes #358

cookeac opened this issue Dec 15, 2022 · 4 comments · Fixed by #367
Assignees
Labels
agenda-next-meeting problem The specification equivalent of a bug to be addressed. this-release Scheduled to be implemented for this release in development

Comments

@cookeac
Copy link
Collaborator

cookeac commented Dec 15, 2022

There are some changes to version 1.3 that require explanation and some changes to code from version 1.2.

  1. Standardisation of the discriminator to ResourceType, removing the EventType discriminator and the IdentifierType discriminator
  2. Removing a discriminator from icarIdentifierType entirely because it is always deserialised in the context of a known resource
  3. The postal address change in icarConsignmentType (this may need to be revisited) AnyOf causes code generation problem in icarConsignmentType #357

We agreed that we need release notes to explain what is changing and what to do.

@AndreasSchultzGEA
Copy link
Collaborator

Andrew, what has to be contributed by Alexey and me?

@cookeac
Copy link
Collaborator Author

cookeac commented Dec 15, 2022

I'll start a release notes outline, and you may be able to improve the explanation of what to do from a programmer level.

@cookeac
Copy link
Collaborator Author

cookeac commented Jan 12, 2023

Proposed text:

Important changes from ADE v1.2.x

There are some important changes that particularly affect OpenAPI code generation between ADE 1.2.x and ADE 1.3.x and later.
If you have implemented ADE 1.2.x or earlier, and particularly if you have used OpenAPI code generation in languages such as C# and Java, you should be aware of these changes.

Standard Discriminator for resources is now ResourceType

The OpenAPI specification allows you to specify a "discriminator" which is used to support de-serialisation of objects from JSON text to native in-memory objects. ADE 1.2.x did not implement this consistently, primarily due to limitations in version 5.x of the common openapi-generator tool.

You can use a discriminator to differentiate, and help to de-serialise JSON into the correct concrete classes (a concept in object-oriented languages like C# and Java). For instance, if you read from a data source that may contain icarResource objects, you want to know whether the actual object being read should be instantiated as an icarTreatmentEventResource.

The OpenAPI rules for discriminators are:

  • The same discriminator must be used in the base class and derived classes
  • The discriminator must be declared as a string property
  • The discriminator is required

We had discriminators in ADE 1.2.x, but these were not declared as string properties (only as a discriminator) and we used different discriminators for Resources (resourceType) and Events (eventType). However, events are also resources.

Version 5.x of openapi-generator does not work properly if the discriminator is defined as a property and required. This does not match the OpenAPI specification.

We identified this when specifying the generic data exchange API where being able to de-serialise objects correctly is essential. We found that some other code generators also did not correctly generate code with our previous specification of discriminators.

Changes made in ADE 1.3.x:

  • The discriminator resourceType is defined as a string property in icarResource.json. It is therefore included in all resources, including events, using "allOf".
  • resourceType is a required field as is required by the OpenAPI specification.
  • eventType has been removed from events as it will not function properly as a discriminator.
  • identifierType has been removed from icarIdentifierType.json. Identifiers are always embedded in known contexts in JSON objects - there is never a need to make a decision when de-serialising these. Further, the only reason why identifiers are subclassed is to assist implementers to understand, find, or define the correct schemes in the well-known identifier schemes.

Changes to your implementations from ADE 1.2.x

  • Regenerate your code using openapi-generator 6.x or later
  • Don't use eventType or identifierType in your implementations
  • Let your data interchange partners know. Most implementations prior to 1.3.x won't actually need to use the discriminator because the API context defines the type of data to be de-serialised. However, having resourceType as a required field may cause problems. We note that as a discriminator in the base class, it was technically always required by the OpenAPI spec, but this may not have been noted and implemented.
  • This is also a reminder to continue using the tolerant reader pattern described in the ADE design principles, which helps insulate from many of these changes.

@cookeac cookeac added this-release Scheduled to be implemented for this release in development problem The specification equivalent of a bug to be addressed. agenda-next-meeting labels Jan 12, 2023
@cookeac
Copy link
Collaborator Author

cookeac commented Jan 12, 2023

Approved this change to be made at our meeting 12 Jan 2023.

cookeac added a commit that referenced this issue Feb 15, 2023
Tested changes for ADE 1.3.2
This pull request integrates the following changes:
* Repairs to the `icarStatisticsResource` and related resources - resolves #353 
* Revert 1.3 changes to `icarConsignmentType` address fields - resolves #357 
* Added an explanation for ADE 1.3 changes around discriminators - resolves #358 
* Testing C# and Java code generation for this patch release - resolves #364
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda-next-meeting problem The specification equivalent of a bug to be addressed. this-release Scheduled to be implemented for this release in development
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants