search

erasmus-without-paper / ewp-specs-api-iias

Specifications of EWP's Interinstitutional Agreements API.
MIT License
4 stars 13 forks source link

get-response.xsd and terminated-as-a-whole #172

Open demilatof opened 5 months ago

demilatof commented 5 months ago

Today, during a session test, I realized that the get-response.xsd documents the terminated-as-a-whole attribute in the following way:

If given MUST be set to "true".

It is not a great problem, but I had to modify my code from

coopCon.setTerminatedAsAWhole(iia.isTerminatedAsAwhole());

to

if (iia.isTerminatedAsAwhole()) coopCon.setTerminatedAsAWhole(iia.isTerminatedAsAwhole());

Just to satisfy a useless specification that has little if not at all sense. The XSLT already does the dirty work, omitting to consider terminated-as-a-whole if it is not present or set to false. In this way the text-to-hash and the consequent IIA-hash change only if terminated-as-a-whole is set to true.

I think it could be better omitting strict rules in the annotation of an XSD if they are not necessary; they could only introduce difficulties in making compatible implementations.

terminated-as-a-whole is an optional attribute, self descriptive and with a clear objective: if set to true it means that the IIA is terminated as a whole. It is not an element used as a placeholder, relevant thanks to its presence, but an attribute relevant thanks to its value.

I suggest to remove the sentence

If given MUST be set to "true".

Nothing should change in the provider implementations, I hope that they don't consider it as true only because it is present without reading its value.

Anyway now I added the redundant "if" and the problem is gone, but I'm afraid that the above requirement could cause issues of interoperability. E.g.: today my IIA wasn't approved because it was not identical to the partner one (old and sad story...) and because it contained terminated-as-a-whole="false" whilst the attribute was absent in the coupled IIA. On the contrary, yesterday another IIA has been approved even if it was in the same situation.

mkurzydlowski commented 5 months ago

This is an optional parameter and on purpose, because very few agreements will be "terminated as a whole". So we don't need this attribute to appear for almost all contracts. Hence the requirement that people not provide it for every IIA and in almost all cases set it to false. Once it exists, it must have some value and therefore it must be "true".

But yes, this means you have to add an if, as a server.

demilatof commented 5 months ago

This is an optional parameter and on purpose, because very few agreements will be "terminated as a whole". So we don't need this attribute to appear for almost all contracts. Hence the requirement that people not provide it for every IIA and in almost all cases set it to false. Once it exists, it must have some value and therefore it must be "true".

But yes, this means you have to add an if, as a server.

But doing so is conceptually wrong; who wants, you should be free to add it and set to false, and everything must be fine. Otherwise you have to declare the attribute as fixed in the XSD and not in the annotation with a "MUST"

<xs:attribute name="terminated-as-a-whole" type="xs:boolean" use="optional" fixed="true">

mkurzydlowski commented 5 months ago

You are right, we should have used the fixed attribute instead of the description.

sascoms commented 4 months ago

@mkurzydlowski is there a reason why the "terminated-as-a-whole" is placed under conditions and not in the iia metadata section?

Does not this element flag all the iia as terminated?

https://github.com/erasmus-without-paper/ewp-specs-api-iias/blob/a151f579ba2278dfbb99d19f40cff18212425e1d/endpoints/get-response.xsd#L232

mkurzydlowski commented 4 months ago

I think the reason was to make it part of the hash calculation.

sascoms commented 4 months ago

So, as now all the iia is hashed, it can be placed in the meta section in future releases? Correct?

mkurzydlowski commented 4 months ago

Not all of the IIA is hashed. Only the iia-id has been added outside of the cooperation-conditions element.

But yes, this attribute might be moved elsewhere and added to the hash calculation in the future.

demilatof commented 4 months ago

I think the reason was to make it part of the hash calculation.

When we discussed about the terminate-as-a-whole, there wasn't yet the XSLT, therefore it could be hashed only the whole cooperation-conditions element and nothing else. It was even discussed if it was better using an attribute or an element inside the cooperation-conditions.