Skip to content

Introduce new icarReproSireInfoType, for the basic sire properties.#590

Open
AlexeyHardCode wants to merge 1 commit intoadewg:sirePrimaryBreed-in-inseminationfrom
mtech-ashelepaev:sirePrimaryBreed-in-insemination
Open

Introduce new icarReproSireInfoType, for the basic sire properties.#590
AlexeyHardCode wants to merge 1 commit intoadewg:sirePrimaryBreed-in-inseminationfrom
mtech-ashelepaev:sirePrimaryBreed-in-insemination

Conversation

@AlexeyHardCode
Copy link
Collaborator

@AlexeyHardCode AlexeyHardCode commented Feb 11, 2026

Due to missing permissions in the adewg repository, I created the PR against Andrea's original branch (adewg:sirePrimaryBreed-in-insemination).

Could you please review the PR @cookeac @erwinspeybroeck @AndreasSchultzGEA?
Once it’s approved, we can proceed with merging the parent PR into Develop.

@AndreasSchultzGEA
Copy link
Collaborator

AndreasSchultzGEA commented Feb 11, 2026 via email

Copy link
Collaborator

@AndreasSchultzGEA AndreasSchultzGEA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the changes seem to be okay.
But an replacement will produce a breaking change.
Therefore, a rollout with 1.5.2 or 1.6.0 is not recommended.

"type": "string",
"description": "URI to an AnimalCoreResource for the donor dam."
},
"sireIdentifiers": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this properties are replaced, there will be a breaking change!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndreasSchultzGEA Thanks for raising this! I totally get why moving the properties into a common type can look like a breaking change at first glance.
If we focus on the actual JSON payloads, nothing really changes. The same properties are still there, at the same level, with the same names and types. From the consumer side, the payload looks exactly the same as before.
So any previously generated code that expects these properties in each schema should continue to work without issues, since the serialized JSON structure hasn’t changed.
Where it might feel like a breaking change is on the code generation side, since the schema structure changes and generators will produce slightly different models. But both the old and the new generated code versions still support the same JSON payload structure. So in terms of the actual data contract, nothing breaks.

That’s why I’d consider this more of a schema refactoring than a real breaking change.

Image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it is a schema refactoring and not a breaking change to the schema. But to Andreas' point, how substantial are the changes to the generated code in C# and Java? Because if developers have to change a lot of their code that references the modified classes, it is still a breaking change for them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your very valuable comments.

To summarize, there are three levels of compatibility we need to consider:

  1. JSON payload – non-breaking → minor version
    The JSON payload remains unchanged — same fields, same structure — so the wire contract is fully backward compatible.

  2. Generated API client/server code – non-breaking → minor version
    The generated API code continues to support the same payload, so from an API usage perspective it also remains backward compatible.

  3. Application internal logic that depends on generated models – potentially breaking → major version
    If an application’s internal logic depends directly on the previously generated model structure (for example, specific class hierarchy, inheritance, database mappings, or integrations with other components), then regenerating the models from the updated schema could introduce breaking changes at compile time. In that scenario, the impact would be considered a major change for those consumers.

@AndreasSchultzGEA you are refering to the third level, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexeyHardCode Yes, I do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndreasSchultzGEA yes, I fully agree that we should always keep this in mind. So, you propose to keep this refactoring ouside this minor update?
@cookeac I’m wondering what level of compatibility guarantee ICAR ADE itself is expected to provide. Have we formally defined this somewhere?

From my perspective, the scope of compatibility for ICAR ADE updates could be structured as follows:

  1. JSON payload: The external data structure must remain stable — same fields, same structure, same semantics.
  2. Spec-level compatibility: Schema validity must be preserved, including consistent required/optional rules, types, and constraints.
  3. Code generation: At minimum, commonly used generators (e.g., C#, Java) should continue to produce usable output that correctly accepts and produces the same payloads.
  4. Consumer internal logic coupling: The most challenging aspect is applications that tightly couple their internal logic to the generated model hierarchy (e.g., database mappings, reflection usage, inheritance assumptions, etc.).
    While we should acknowledge this risk and try to minimize unnecessary disruption, it may not be realistic to block schema refactoring solely because some internal implementations depend on specific model structures.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At our meetings today we agreed that this can be merged once we have some brief documentation to accompany the 1.6 release so that users know that if they regenerate their code from the schema, they may need to change how they reference fields in these objects.
There will not be a breaking change for users who are interacting with another service as the JSON itself is not changed.

@AlexeyHardCode
Copy link
Collaborator Author

The C# code generation also seems to work fine with the changes above. And the final JSON payload still looks the same in the generated openapi.json schema.

@tpekeler
Copy link
Collaborator

We tested the Java generation and that worked fine. Maybe we didn't use the parts which could possibly be altered after generation of new classes. Our Implementation worked without changes.

"type": "string",
"description": "URI to an AnimalCoreResource for the donor dam."
},
"sireIdentifiers": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At our meetings today we agreed that this can be merged once we have some brief documentation to accompany the 1.6 release so that users know that if they regenerate their code from the schema, they may need to change how they reference fields in these objects.
There will not be a breaking change for users who are interacting with another service as the JSON itself is not changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants