new-zealand-research-information-system / nzris-api-specifications

2 stars 1 forks source link

protectionRequirements inefficient with datePatternExpires a real date #9

Open Jason-Gush opened 4 years ago

Jason-Gush commented 4 years ago

1/ Having ProtectionExpiration and datePatternExpires both required consumes a fair bit of bandwidth ( ~4 MB out of 36 MB for a initial omnibus submission) despite being: { "ProtectionExpiration": "No", "datePatternExpires": "1900-01-01", "pattern": "08" }

in all initial cases.

IMHO {"pattern":"08"} should have been sufficient, with the existence of a datePatternExpires value indicating that the protection does in fact expire.

2/ Despite the description for datePatternExpires suggesting an empty value will be dealt with, the validation is string($date), i.e., an actual expiry date must be given even when the date is irrelevant

melissafordyce commented 4 years ago

Hi Jason, thanks for your comment. You are correct, the date is not required for records with no protection expiration. The processing against the data specification (which includes whether a field should have been provided) will happen once the submission is loaded into the Custodian Application - not during the API. Therefore, if fields are optional and do not have a valid value, they can be left blank - regardless of API format. This example you've provided above should not cause an error during ingestion.

Jason-Gush commented 4 years ago

Still this is still an issue in 2.05. NB we're told that having "datePatternExpires" is "No" with an expiry date errors in ingestion. I think what is actually intended is that neither of these fields is required, with the validation happening in the service.

TBC we use the spec to generate a mock of the service (using express-openapi-validator), and test the submission before that. What your spec is doing is forcing structure which the Custodian App won't accept, i.e.,

ProtectionRequirement: required:

But what I think you're really meaning is:

ProtectionRequirement: required:

melissafordyce commented 4 years ago

Hi Jason, The mandatory fields for ProtectionRequirements are pattern and ProtectionExpiration. datePatternExpires is only required if ProtectionExpiration = Yes. In saying that, the Custodian Application won't provide an error if you do provide a date, even if you've said No for ProtectionExpiration.

The structure of the spec should align with the data specification. However, the API itself doesn't validate whether you have provided mandatory data for each entity - this validation is provided in the Custodian Application itself.

Jason-Gush commented 4 years ago

Hi Melissa,

"The structure of the spec should align with the data specification." Yes, this is what I'm talking about.

api-definition.yaml v2.05 is saying that datePatternExpires must be present at all times.

Can the spec please be corrected to match the intention, i.e., what you're saying would be: required:

NB: where these kinds of validation rules cannot be defined in a Swagger 2.0 spec, it would be great to have them spelled out in the descriptions; alternatively, consider offering the spec as OAS3.0 where this kind of conditional relationship can be represented.

melissafordyce commented 4 years ago

Ah my apologies Jason - we will get that updated in the YAML. We will also consider your suggestion above.