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

Removed type 'null' from countryCode #378

Closed
wants to merge 1 commit into from

Conversation

mrshll1001
Copy link
Contributor

@mrshll1001 mrshll1001 commented Feb 20, 2024

countryCode allows for a type of either 'string' or 'null'. Unless there is a specific reason why null is permitted as a countryCode we should amend this.

The semantics of having a type of null in JSON Schema:

https://json-schema.org/understanding-json-schema/reference/null

I did want to flag that this is such an odd choice that it may have been deliberate i.e. if we wanted to accommodate unrecognised territories which are not yet assigned a countryCode.

If accepted, I believe this should represent a PATCH level change however if there is existing data with null as a countryCode it would become invalid.

We may well reject this PR on the basis that it constitutes a breaking change and not a bugfix, which I'm fine with.

countryCode allows for a type of either 'string' or 'null'. Unless there
is a specific reason why null is permitted as a countryCode we should
amend this.

The semantics of having a type of null in JSON Schema:

https://json-schema.org/understanding-json-schema/reference/null
@mrshll1001 mrshll1001 changed the base branch from master to 1.4-staging February 20, 2024 16:23
@mrshll1001 mrshll1001 marked this pull request as ready for review February 20, 2024 16:27
@mrshll1001 mrshll1001 requested a review from KDuerden February 20, 2024 16:27
@KDuerden
Copy link
Contributor

thanks @mrshll1001

if there is existing data with null as a countryCode it would become invalid.

I interpret the rules that breaking changes are changes that have the potential to break data, whether or not actual data would be broken. This should be packaged alongside any other "MAJOR in theory not practice" changes as part of any future version upgrade.

odd choice that it may have been deliberate i.e. if we wanted to accommodate unrecognised territories which are not yet assigned a countryCode.

As this field is a property of the location definition, it gets applied to funding or recipient organisations or beneficiaries. Although it doesn't make much sense in the context of organisations, apart from as an outlet for unrecognised countries, for 'beneficiaries' there is a sense that 'null' mirrors what we did with the location scope codelist, in terms of allowing for a deliberate 'n/a' value.

I'm now curious whether there is any documentation that explains this as a 'bug' or deliberate. Does schema.org or IATI's country code fields include a similar null option which we inherited this from? However, as I believe this change is blocked as a consequence of its status as a breaking change, finding out the answer to this question isn't a high priority right now.

@mrshll1001
Copy link
Contributor Author

Good question.

There's nothing obvious on schema.org which makes me think that we inherited it from there. I can see that Country and a few other objects have an addressCountry property which accepts either a plaintext string e.g. "United Kingdom" or a two-letter ISO 3166-1 country code.

In terms of IATI, I've asked some of our ODS colleagues which work more closely with IATI to see if they have any insight but it looks like they also just state a preference for the country code in the summary tables for each organization and activity for the recipient country specifically. I'm not as fluent in XML Schema as JSON schema but as far as I can tell, there's no allowance for null there either.

I'm going to close this PR as blocked by the fact it's a MAJOR upgrade as it has the potential to break data, and raise an issue to reference the conversation and document it for if/when we do version 2.0.

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.

2 participants