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

Proposal for a Common.ValidationFunction on parameters and action #233

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nlunets
Copy link
Member

@nlunets nlunets commented Feb 24, 2023

You might want to invest in a file formatter since it seems that the line ending are not consistent :(

@ralfhandl
Copy link
Member

We are recommending a VS Code plugin for XML that has nice pretty-printing:

"redhat.vscode-xml"

@nlunets nlunets force-pushed the Common.LocalevalidationFunction branch from 7d4e03f to 50ac28e Compare February 24, 2023 15:29
@nlunets
Copy link
Member Author

nlunets commented Feb 24, 2023

I'm not usually using vscode though so that's my problem.
Fixed it here

@ralfhandl
Copy link
Member

I'm not usually using vscode though so that's my problem.

We could try out https://www.npmjs.com/package/@prettier/plugin-xml and force-format XML files during npm run build. Need to try out whether this conflicts with the Red Hat XML plugin.

The Red Hat XML plugin provides XML schema validation and schema-based auto-complete, so maybe you want to switch to VS Code when contributing to this repository?

@nlunets
Copy link
Member Author

nlunets commented Feb 24, 2023

Having prettier running could also help for sure but that's really up to you !
Otherwise yes i'm now using vscode for this one :)

nlunets and others added 2 commits February 24, 2023 17:03
Co-authored-by: Heiko Theißen <heiko.theissen@sap.com>
"$AppliesTo": ["Parameter", "Action", "Function"],
"@Common.Experimental": true,
"@Core.Description": "Function that validates whether the parameter value, or parameter values (in case of Action and Function) is correct or not",
"@Core.LongDescription": "This annotation must not target a binding parameter. When it targets a non-binding parameter, the validation function has one parameter to which the to-be-validated parameter value must be passed. When this annotation targets an action or function, the validation function has one parameter per non-binding action/function parameter to which the to-be-validated value must be passed. The validation function returns true or false."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not also require the binding parameter to have the right context? E.g. if the bound action changes the status of an object, the allowed new statuses might depend on the current status of the object.
From another angle: What about overloading? Or would overloading not be allowed because the ValidationFunction is unbound?

<Annotation Term="Common.Experimental" />
<Annotation Term="Core.Description" String="Function that validates whether the parameter value, or parameter values (in case of Action and Function) is correct or not" />
<Annotation Term="Core.LongDescription">
<String>This annotation must not target a binding parameter. When it targets a non-binding parameter, the validation function has one parameter to which the to-be-validated parameter value must be passed. When this annotation targets an action or function, the validation function has one parameter per non-binding action/function parameter to which the to-be-validated value must be passed. The validation function returns true or false.</String>
Copy link
Contributor

@HeikoTheissen HeikoTheissen Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<String>This annotation must not target a binding parameter. When it targets a non-binding parameter, the validation function has one parameter to which the to-be-validated parameter value must be passed. When this annotation targets an action or function, the validation function has one parameter per non-binding action/function parameter to which the to-be-validated value must be passed. The validation function returns true or false.</String>
<String>When this annotation targets a non-binding parameter, the validation function must have a bound or unbound overload with one non-binding parameter to which the to-be-validated parameter value must be passed. When this annotation targets an action or function overload, the validation function must have an overload with the same parameter types (including binding parameter, if applicable) as the annotation target. The validation function returns true or false.</String>

Copy link
Member Author

@nlunets nlunets Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You lost me as soon as you introduced the word overload here.
Why would overload matter in this case ? We're talking about a function such as

myValidationFunction(parameter:any): 200OK | Error with message which probably need to be defined / aligned

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An action or function can have several overloads with different parameters. And you could annotate individual overloads with different validation functions, or with different overloads of the same validation function.

myValidationFunction(parameter:any): 200OK | Error with message which probably need to be defined / aligned

This is a validation function for one parameter. But as @uhlmannm pointed out, the result of the validation can also depend on other parameters, including the binding parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok in which case if you're targeting a bound action/ function you also need the validation function to exist in relation to the existing binding parameter, make sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment regarding overloads was rather that the addressed action or function could have overloads itself. The typical scenario seems to be that the same name is used for operations with different binding parameters. I wonder whether identifying the right validation function would be easier if the validation function has the same binding parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A validation function needs a binding parameter, if it shall validate more than one parameter of a bound action or function. For validating only one parameter, an unbound validation function is needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see only a limited use in a validation without the binding parameter. A validation from business perspective would often require to know the context.
Also for the UI design I would not see "bound validation functions" as a big restriction. Normally, the user selects the object / binding parameter, then presses the action button and finally fills all the other parameters in a popup. So the binding parameter is available when calling a validation function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see only a limited use in a validation without the binding parameter.

I have updated my suggestion accordingly.

vocabularies/Common.json Show resolved Hide resolved
@@ -1425,6 +1425,14 @@ If the request succeeds the response will contain a JSON body of `Content-Type:
<Annotation Term="Core.Description" String="Base URL for WebSocket connections" />
</Term>

<Term Name="ValidationFunction" Type="Common.QualifiedName" Nullable="false" AppliesTo="Parameter Action Function">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Common.QualifiedName identifies a function. Do we also want to be able to identify a particular function overload?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get how you call this function at all. You need the qualified name for a bound function, but what would be its binding parameter? I assume an unbound function is sufficient, but then you would need the name of the function import to call it. Once this is clarified, we can look at the rules for overloading (https://docs.oasis-open.org/odata/odata-csdl-xml/v4.01/odata-csdl-xml-v4.01.html#sec_FunctionOverloads)

Also: for a single non-binding parameter, the overload should be clear from its type. For an operation, I wonder if the validation function uses the same sequence of parameters or if there is a correlation based on names? Depending on that, there may again be no freedom w.r.t. overloads.

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.

5 participants