Should we disallow extra properties on QNode and QEdge

[edeutsch]

Current TRAPI 1.2 and 1.3 policy permits extra properties on QNode and QEdge. We have stipulated the policy in https://github.com/NCATSTranslator/ReasonerAPI/blob/1.3/ImplementationRules.md that if a server receives a property on a QNode or QEdge that it does not recognize, it "SHOULD generate a warning and MAY continue". This is very lax because we couldn't agree on anything stronger.

There was a question/motion last time on whether we should disallow extra properties here.

What do you think?

[edeutsch]

From the ARAX perspective, our code checks all properties passed by the client on QNode and QEdge and if there are any that we do not recognize, we throw a runtime error and stop processing. Aside from the currently allowed properties, we allow 'name' on QNode for enhanced readability, and we allow 'option_group_id' on both QNode and QEdge to allow alternative paths in a query_graph. We also allow the "exclude" flag on QEdges. I think it is useful to allow flexibility in the QNodes and QEdges. I think our SHOULD and MAY above ought to be stronger to MUST and MUST NOT, respectively. But I do not think we should make it illegal TRAPI to add additional features.

[ehinderer]

I think the decision depends on what value the "extra properties" are adding. Sorry I'm a little hazy on the details here, are these extra properties in the Qnode and Qedge meant to constrain the query? If so, why are they separate from constraints? I'm assuming these extra properties are KP-specific, and therefore the query graph designer needs KP-specific awareness in defining those properties. Is this right?

If a query gets through that correctly utilizes unique KP attributes, and an ARA is aware of these and can correctly reason over them, then it shouldn't be prevented from doing so by virtue of extra properties being disallowed. That being said, properties that are (in my view) "secret" or "hidden" in this way is probably not a good thing. Maybe a better design would be to keep the constraints up-to-date with all properties that KPs provide? This relies on my above assumptions on the topic being correct. All properties are known; but not all ARAs can reason across all properties.

I agree that if an ARA does not "understand" the extra property in question it shouldn't process it as if it does. In that respect I agree that the rules should be stronger i.e., "MUST and MUST NOT" as above. An inventory of all properties, that ARAs have access to, would guarantee that ARAs at least recognize the property.

[uhbrar]

From the ARAGORN perspective, we allow the use of allowlistlist and denylist, as discussed before. We do not accept any other properties in the edge, although we do not stop processing if additional properties are present. If there are any properties that we do not support, we don't touch them. As long as it fits within the defined TRAPI model, we accept the query.

[edeutsch]

Please respond to an emoji poll to get a sense of what people are thinking. Not a final binding decision.

A) (thumbs UP) We should CHANGE the TRAPI schema to additionalProperties: FALSE for QNode and QEdge

B) (thumbs DOWN) We should just LEAVE the schema as is: additionalProperties: true for QNode and QEdge

[edeutsch]

At the TRAPI call on 5/19, we concluded that we will defer any decision on disallowing extra properties on QNode and QEdge to TRAPI 1.4. Will not be considered further for 1.3.