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

AnyOf causes code generation problem in icarConsignmentType #357

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

AnyOf causes code generation problem in icarConsignmentType #357

cookeac opened this issue Dec 15, 2022 · 10 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

The issue is this clause:

    "originAddress": {
      "description": "Origin address for movement.",
      "anyOf": [
          { "type": "string" },
          { "$ref": "../types/PostalAddress.json" }
      ]
    },

This causes code generation to ONLY generate the PostalAddress version, which is a breaking change from previous versions where only a string was possible. This occurs with openapi-generator v6.

@cookeac
Copy link
Collaborator Author

cookeac commented Dec 15, 2022

@AlexeyHardCode pointed out that openapi-generator 6.x generates code that correctly handles this in C#, BUT in an implementation assignments to or reading from originAddress needs to change slightly,
@AndreasSchultzGEA does not think this is as well handled in Java.

We need to decide whether this should be handled as a slightly breaking change with documentation, or reverted so that originAddress and destinationAddress are only strings. If we need to revert, then it would be good to do this in the 1.3.2 patch.

@cookeac
Copy link
Collaborator Author

cookeac commented Dec 15, 2022

@AndreasSchultzGEA if you could post a piece of the generated code for originAddress in Java, please, that might help us with our assessment of how significant is the breaking change.

@AndreasSchultzGEA
Copy link
Collaborator

AndreasSchultzGEA commented Dec 15, 2022

    name = "icarConsignmentType",
    description = "Consignment information for a movement (arrival, departure)."
)
@JsonTypeName("icarConsignmentType")
public class IcarConsignmentType {
    @JsonProperty("id")
    private IcarIdentifierType id;
    @JsonProperty("originLocation")
    private IcarLocationIdentifierType originLocation;
    @JsonProperty("originAddress")
    private IcarConsignmentTypeOriginAddress originAddress;
    @JsonProperty("destinationLocation")
    private IcarLocationIdentifierType destinationLocation;
    @JsonProperty("destinationAddress")
    private IcarConsignmentTypeDestinationAddress destinationAddress;
...

@AndreasSchultzGEA
Copy link
Collaborator

AndreasSchultzGEA commented Dec 15, 2022

@Schema(
    name = "icarConsignmentType_originAddress",
    description = "Origin address for movement."
)
@JsonTypeName("icarConsignmentType_originAddress")
public class IcarConsignmentTypeOriginAddress {
    @JsonProperty("addressCountry")
    private String addressCountry;
    @JsonProperty("addressLocality")
    private String addressLocality;
    @JsonProperty("addressRegion")
    private String addressRegion;
    @JsonProperty("postOfficeBoxNumber")
    private String postOfficeBoxNumber;
    @JsonProperty("postalCode")
    private String postalCode;
    @JsonProperty("streetAddress")
    private String streetAddress;

    public IcarConsignmentTypeOriginAddress() {
    }
... 

@AndreasSchultzGEA
Copy link
Collaborator

There is no additional Constructor taking a String-Argument created as it seems to be present at C#.

@cookeac
Copy link
Collaborator Author

cookeac commented Jan 11, 2023

Here's my suggestion:

  • We added the AnyOf PostalAddress option to help those who needed to transfer structured address data that the previous string did not provide. In theory it would have allowed simple strings to be used as well but this doesn't work with Java codegen.
  • As I've started specifying a Carcass data event based on the Australian specifications, it would be helpful to have other information about the source or destination facility (including organisational identifiers, tax code, etc).
  • I propose that in the short term (version 1.3.2 update) we revert the originAddress and destinationAddress to string.
  • Then in version 1.4, we should add a separate, optional, originOrganization and destinationOrganization (US spelling) which could carry additional organisation information as well as the PostalAddress. As it is a separate attribute this won't cause the same code generation issues.

@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 11, 2023
@AndreasSchultzGEA
Copy link
Collaborator

reverting the addresses from 1.3 back to 1.2-version implies to change the code again...
Don't all the companies we know already have changed their implementation? Would they like to adjust it again?

If we decide to revert/ change, why in 2 steps instead of one?

@AndreasSchultzGEA
Copy link
Collaborator

I'll take this issue to the issue-tracker of OpenAPIGenerator

@cookeac
Copy link
Collaborator Author

cookeac commented Jan 12, 2023

During our discussion on 12 Jan 2023 we discussed:

  1. Identifying a bug in openapi-generator for Java. This could then be addressed with a workaround such as an openapi-generator option (if available) or manually patching Java code after generation.
  2. Reverting the definition of originAddress and destinationAddress to strings (ADE 1.2) and adding more complex organisation objects as proposed in Extensions to Consignment for animals being sent to a processor #359. However we are concerned about introducing more changes to the 1.3.2 patch.
  3. Reverting the definition of originAddress and destinationAddress to strings (ADE 1.2) and adding an originPostalAddress and destinationPostalAddress with the structured address. This avoids a breaking change and the code generation problem. It is slightly less elegant but may be better for users.
    In the meantime, Andreas will follow up as noted above.

@cookeac
Copy link
Collaborator Author

cookeac commented Feb 9, 2023

On 09 February 2023 we agreed:

  • Reverting the definition of originAddress and destinationAddress to strings (ADE 1.2) and marking these as deprecated.
  • Adding an originPostalAddress and destinationPostalAddress with the structured address.

This avoids a breaking change and the code generation problem. It is slightly less elegant but may be better for users.

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.

2 participants