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

Request to add "Unknown" to icarDepartureReasonType enumeration #206

Closed
DavidJeppesen-Seges opened this issue Mar 19, 2021 · 14 comments · Fixed by #227
Closed

Request to add "Unknown" to icarDepartureReasonType enumeration #206

DavidJeppesen-Seges opened this issue Mar 19, 2021 · 14 comments · Fixed by #227
Labels
enhancement New feature or request this-release Scheduled to be implemented for this release in development

Comments

@DavidJeppesen-Seges
Copy link

We would like to have "Unknown" added to the list of enum values to cover the case of missing data.
We are aware that "Other" already exists and we did consider whether it could cover this case, but there still is a semantical difference between the two, while "Other" excludes all current values in the enum and "Unknown" does not.

@DavidJeppesen-Seges DavidJeppesen-Seges changed the title Proposal to add "Unknown" to icarDepartureReasonType enumeration Request to add "Unknown" to icarDepartureReasonType enumeration Mar 23, 2021
@ahokkonen ahokkonen added the enhancement New feature or request label Mar 25, 2021
@ahokkonen
Copy link
Contributor

One of the solution would be to pass "null" for "Unknown" indication.
But if this information is required explicitly I don't mind to have a separate value for this status.

@cookeac @alamers @erwinspeybroeck what other think about that?

@cookeac
Copy link
Collaborator

cookeac commented Mar 25, 2021

I felt that as the field is not mandatory, null or not specified would be acceptable. However, it would be useful to know if this was a regulated field in Denmark's animal movement systems, in which case a specific value would be justifiable.

@DavidJeppesen-Seges
Copy link
Author

What do you mean by "regulated" @cookeac?

I agree that null would be acceptable, but I think Unknown would be better regarding communication - for the reason supplied here: #199 (comment)

If it is decided to make the enum nullable, null should be added to the list of values as specified in OAS3 under "Nullable enums".

@ahokkonen
Copy link
Contributor

In our technical HUB implementation we define almost every property as a nulluble, to prevent default initiation of the type (like numeric, dates) during serialization if the field value was not specified. In my opinion this would be good to define in a OAS specs as well for potential nullable properties.

@cookeac
Copy link
Collaborator

cookeac commented Mar 25, 2021

There are many fields that could potentially have null values. I'm still not convinced by the linked comment - I don't see why we should add Unknown to every enumeration. It made sense for Sex because that was a required field, and there is an explicit case where substituting another non-null value would be incorrect.

A field being nullable allows organisations to decide any of - can I provide a value here? Do I know the value? Is a value here important and appropriate?

I don't think the JSON schema spec supports the OAS3 extension (nullable enums). I would be interested in some documentation otherwise (and would be happy for us to implement) as I couldn't find this in the JSON Schema specification, and these JSON schemas could be used for other than Swagger APIs.

@ahokkonen
Copy link
Contributor

Only when this information is required on receiving party and "null" could be considered as a incomplete event data.
Leaving service to decide whether null is an indication of "Unknown" or wrong data leak.

@DavidJeppesen-Seges
Copy link
Author

It seems to me that section 6.1.2 in the JSON schema specs allows for null to be defined in the array of possible enum values?

My (very down-to-earth) problem regarding this is, that when I autogenerate the models as C# classes from the specification, the enum on the model is not nullable, so when we have no data to map to an icarDepartureReasonType, I'm forced to choose "Other", which is not semantically correct for the reason I mentioned above.
I could of course, manually, alter the class, but that will not behave nicely if clients also are autogenerating from the specification.

@cookeac
Copy link
Collaborator

cookeac commented Mar 25, 2021

Only when this information is required on receiving party and "null" could be considered as a incomplete event data.
Normally I'd argue that this is a design fault on the behalf of a receiving party insisting that a field was required when it might not be able to be observed/measured/known.
However @DavidJeppesen-Seges explanation of the C# autogeneration is a good justification -- even though it is uncomfortable from a pure data modelling sense, where we've already made sure the field is not mandatory.

There are a lot of enumerations. Before we add "null" to each, should we do a little investigation to see if there is a C# codegen option that would address this? Otherwise, I think we should add null to each non-required enumeration just to address this.

@DavidJeppesen-Seges
Copy link
Author

Good idea @cookeac. :) I will have a look.

@DavidJeppesen-Seges
Copy link
Author

DavidJeppesen-Seges commented Mar 31, 2021

After doing some research, this is what I found out:

  1. This issue is discussed in both the OAS GitHub and the NJsonSchema GitHub. The nullable property is apparently being replaced and added as an extra type instead in OAS 3.1.
  2. I have looked into the NSwag code generator as well and unfortunately neither the Open API Generator Tool or NSwag provides an option to make a non required enum nullable when generating C# code.

Further, I tried out both tools on the specification and the Open API Generator Tool insists on making the enum non-nullable - even when I add "nullable": true and null to the list of values. The NSwag generator do make the enum nullable with these additions.

A thorough presentation of the issue is provided in an issue on the Open Api Generator GitHub and I think that his suggestions here makes sense - adding null as a type in a type array and/or the oneOf property.

As there is still some discussions going on how to solve this across OAS, JsonSchema and various code generator forums, I will suggest the smallest change possible for now. Apparently, the OA Generator is not currently able to generate the nullable enums I need, but the NSwag generator only needs null to be added to the array of values to generate a nullable enum, so that is my suggestion.

Thoughts on this @cookeac?

@DavidJeppesen-Seges
Copy link
Author

I'm currently looking into enabling Open API Generator to make nullable enums. It looks like using NSwag will introduce other issues or at least demand extra configuration efforts.

@DavidJeppesen-Seges
Copy link
Author

DavidJeppesen-Seges commented Apr 6, 2021

No matter what I do - I can't make the Open API Generator generate a nullable enum and I see issues on their GitHub, that relates closely to this problem.
I have created an issue on their GitHub.
Provided that ADE will be relying on the Open API Generator (which I'm assuming), maybe it's best to see what they come up with and for now, developers must modify the generated models to fit the specs.

What do people think?

@erwinspeybroeck
Copy link
Collaborator

#214 is the issue for the nullable problem

@cookeac
Copy link
Collaborator

cookeac commented Jul 15, 2021

Agreed to add Unknown to the enumeration. @cookeac to add to a PR

@cookeac cookeac added the this-release Scheduled to be implemented for this release in development label Jul 15, 2021
cookeac added a commit to cookeac/ICAR that referenced this issue Jul 26, 2021
* Added "Unknown" to Departure Reason. Resolves adewg#206.
* Added x/y/z positions to QuarterMilking, and TEMPERATURE to milk characteristics. Resolves adewg#170.
cookeac added a commit to cookeac/ICAR that referenced this issue Jul 26, 2021
* Added "Unknown" to Departure Reason. Resolves adewg#206.
* Added x/y/z positions to QuarterMilking, and TEMPERATURE to milk characteristics. Resolves adewg#170.
@cookeac cookeac closed this as completed Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request this-release Scheduled to be implemented for this release in development
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants