-
Notifications
You must be signed in to change notification settings - Fork 37
Introduce new icarReproSireInfoType, for the basic sire properties. #590
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
Open
AlexeyHardCode
wants to merge
1
commit into
adewg:sirePrimaryBreed-in-insemination
Choose a base branch
from
mtech-ashelepaev:sirePrimaryBreed-in-insemination
base: sirePrimaryBreed-in-insemination
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+70
−96
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| { | ||
| "description": "Describes sire information related to a reproductive event.", | ||
|
|
||
| "type": "object", | ||
|
|
||
| "properties": { | ||
| "sireIdentifiers": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "../types/icarAnimalIdentifierType.json" | ||
| }, | ||
| "description": "One or more unique scheme/identifier combinations for the sire." | ||
| }, | ||
| "sireOfficialName": { | ||
| "type": "string", | ||
| "description": "Official herdbook name of the sire." | ||
| }, | ||
| "sireURI": { | ||
| "type": "string", | ||
| "description": "URI to an AnimalCoreResource for the sire." | ||
| }, | ||
| "sirePrimaryBreed": { | ||
| "$ref": "../types/icarBreedIdentifierType.json", | ||
| "description": "ICAR Breed code for the sire. The usage of this property is not reasonable if more than one sire is provided." | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,42 +1,34 @@ | ||
| { | ||
| "description": "Gives one possible sire recommended to use on an animal.", | ||
|
|
||
| "type": "object", | ||
|
|
||
| "properties": { | ||
| "recommendationType": { | ||
| "$ref": "../enums/icarRecommendationType.json" | ||
| }, | ||
| "sireIdentifiers": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "../types/icarAnimalIdentifierType.json" | ||
| }, | ||
| "description": "Unique scheme/identifier combinations for the sire, including official ID and Herdbook." | ||
| }, | ||
| "sireOfficialName": { | ||
| "type": "string", | ||
| "description": "Official herdbook name of the sire." | ||
| }, | ||
| "sireURI": { | ||
| "type": "string", | ||
| "description": "URI to an AnimalCoreResource for the sire." | ||
| "allOf": [ | ||
| { | ||
| "$ref": "../types/icarReproSireInfoType.json" | ||
| }, | ||
| "isSexedSemen": { | ||
| "type": "boolean", | ||
| "description": "True if this is sexed semen." | ||
| }, | ||
| "sexedGender": { | ||
| "$ref": "../enums/icarAnimalGenderType.json", | ||
| "description": "Specify Male or Female for sexed semen only." | ||
| }, | ||
| "parity": { | ||
| "type": "integer", | ||
| "description": "The parity of the cow for which the recommendation is valid." | ||
| }, | ||
| "sireRank": { | ||
| "type": "integer", | ||
| "description": "The rank of the sire in the recommendation, 1 = first choice, 2 = second, ...." | ||
| { | ||
| "type": "object", | ||
|
|
||
| "properties": { | ||
| "recommendationType": { | ||
| "$ref": "../enums/icarRecommendationType.json" | ||
| }, | ||
| "isSexedSemen": { | ||
| "type": "boolean", | ||
| "description": "True if this is sexed semen." | ||
| }, | ||
| "sexedGender": { | ||
| "$ref": "../enums/icarAnimalGenderType.json", | ||
| "description": "Specify Male or Female for sexed semen only." | ||
| }, | ||
| "parity": { | ||
| "type": "integer", | ||
| "description": "The parity of the cow for which the recommendation is valid." | ||
| }, | ||
| "sireRank": { | ||
| "type": "integer", | ||
| "description": "The rank of the sire in the recommendation, 1 = first choice, 2 = second, ...." | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
JSON payload – non-breaking → minor version
The JSON payload remains unchanged — same fields, same structure — so the wire contract is fully backward compatible.
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.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexeyHardCode Yes, I do.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.