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

"Unknown" to be added in icarAnimalGenderType #199

Closed
dirbSEGES opened this issue Feb 26, 2021 · 10 comments
Closed

"Unknown" to be added in icarAnimalGenderType #199

dirbSEGES opened this issue Feb 26, 2021 · 10 comments

Comments

@dirbSEGES
Copy link

Hi,

We are missing the opportunity to select "unknown" in gendertype.

in case of stillborn we have many registrations with unknown gender.

In generel we are missing the "unknown" (or NULL) opportunity in the Enum collection

BR

@cookeac
Copy link
Collaborator

cookeac commented Feb 26, 2021 via email

@DavidJeppesen-Seges
Copy link

DavidJeppesen-Seges commented Feb 26, 2021

Hello Andrew

Thankyou for fast reply. :)
If you allow null, we still need the enums to be defined nullable according to the OAS3 definition under "Nullable enums".

It should have nullable: true and null defined in the list of enum values.

I understand that it is weird having null in ie. the gender enum, but if you don't take incomplete data into account, I only see 2 possible implementations for data providers:

  1. They will choose a default from the values available (ie. missing gender will result in default male).
  2. They will emit a null value anyways and break the specification. Aside from contradicting the purpose of a specification, this will lead to increased implementation- and maintenance-times.

Whether the enums are made nullable or an "unknown" enum is added doesn't matter to us as long as the case of missing data can be represented.
I sincerely hope you will reconsider. :)

BR David

@ghost
Copy link

ghost commented Feb 26, 2021

I think the point here is that the "gender" field on the animalCoreResource is mandatory. Hence, leaving the field out is not an option. Setting it to null would result in an error, I believe, we'll indeed have to allow null values there or make the field not mandatory.

@ahokkonen
Copy link
Contributor

ahokkonen commented Feb 26, 2021

I think the point here is that the "gender" field on the animalCoreResource is mandatory. Hence, leaving the field out is not an option. Setting it to null would result in an error, I believe, we'll indeed have to allow null values there or make the field not mandatory.

Yes, exactly, there is not a problem to define fields as nullable while implementing the standard, but would this then conflict with ADE definitions?
In our ADE service we declare mostly everything as nullable, because different clients has different requirements, but mandatory field are not.

Also an animal identifier should not be a mandatory while registering parturition, while for stillborn animal it is not required.

@cookeac
Copy link
Collaborator

cookeac commented Mar 2, 2021

The previous (XML-based) ADE specification had gender as a mandatory field, with values Male and Female.

We brought that across into the JSON specification, and added MaleNeuter, FemaleNeuter, MaleCryptorchid, but we did not cater for Unknown (interestingly enough, the schema we took these additional values from, datalinker.org DID support unknown). The case of progeny listed in a parturition event is a valid case for unknown gender.

It seems there are two solutions to the gender problem:

  1. Remove gender from the required[] array in icarAnimalCoreResource. This will allow gender to be unspecified.
  2. Add null to the enumeration values in icarAnimalGenderType. Note that JSON Schema does not appear to have the "nullable: true" function that is required when you are using Swagger YAML.

I believe the first of these options is easier, but will require documentation. Comments?

The second issue is that when icarAnimalCoreResource is reused in the array of progeny for an icarReproParturitionEventResource, we might not have an identifier to provide if the animal is untagged.

I can see three possible solutions (there may be more):

  1. Remove identifier from the required[] array in icarAnimalCoreResource. However, in all other cases that I can see, identifier should be required, and making it optional might be misleading.
  2. Change out icarAnimalCoreResource for a similar but more optional class in icarReproParturitionEventResource. This would be a breaking change.
  3. Remembering that within an icarIdentifierType the scheme and ID attributes are not mandatory, define a scheme that can be used to indicate an unidentified animal, and document this use. For instance, "org.icar.not-identified".

I'm not sure about any of these. The third item arguably avoids breaking changes and unintended side effects, but would require documentation and consistency of use! Thoughts from others?

@DavidJeppesen-Seges
Copy link

I have another comment on the issue with gender. In my opinion, adding an "Unknown" enum may be the best solution regarding communication, as it clearly specifies why there is no value and when it is okay to omit it.
Making it optional or nullable could be interpreted as "not needed" and could cause data suppliers to omit data even when they have it.

The documentation could help explain what nullable or optional really means, but I think it would be safer to have the enum describe it too and it forces developers to make an active decision.

@ghost
Copy link

ghost commented Mar 2, 2021

As for the first part, I'd also vote for adding "Unkown" to the enum. This should be a non-breaking change for existing communication. Newer clients could make trouble, of course, but we have the version identifier on the spec for that. So it isn't really backwards compatible, but at least it's just an extension that you can implement between to parties whenever you are ready.

As for the reproParturitionEventResource: IMHO for a simple solution I wouldn't change anything at all. I think putting the burden onto the systems to generate an additional DB id for this animal is OK. Yes, that means another dead entry in the database, but there will be many anyway.
I'm less sure about the "right" way. We could add another field that signals "no animal recorded" and make the animal optional. In general, I don't really like having the animal in the event at all. A reference would be sufficient for me (and the same goes for other events). I'd rather add the animal directly to a system and then afterwards add the event that references the already stored animal. Still, that would require the option to set "no animal recorded" at least for the parturition event.
Having the (required) "flag" also makes it easy to document: "if 'no animal recorded' is set to false, the animal field has to be filled". However, this puts an additional validation burden onto the systems. Can that be expressed in JSON schema? I guess yes, but do we want that?

@ahokkonen
Copy link
Contributor

I would vote for adding "Unknown" as a new value for icarAnimalGenderType -type.

@erwinspeybroeck
Copy link
Collaborator

I think most I&R systems are using a STILLBIRTH event to have this reported.
In the stillbirth event sex is not mandatory

@ahokkonen
Copy link
Contributor

New value is added to enumeration in PR#204 , closing this ticket.

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

No branches or pull requests

5 participants