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

Add injectArgs option to @can directive to pass along client defined arguments to the policy check #1043

Merged
merged 11 commits into from
Nov 14, 2019

Conversation

faiverson
Copy link
Contributor

@faiverson faiverson commented Nov 12, 2019

  • Added or updated tests
  • Added Docs for all relevant versions

Resolves #1002

Changes
Added a new argument to avoid breaking changes with the current version.
Saying that I think args (getAdditionalArguments) is always a plain string, it doesn't have to much sense to me

@spawnia
Copy link
Collaborator

spawnia commented Nov 12, 2019

Can you clarify the semantics of what happens if the statically defined args and the injected arguments collide? That should be backed up by a test.

@lorado
Copy link
Collaborator

lorado commented Nov 12, 2019

Can you clarify the semantics of what happens if the statically defined args and the injected arguments collide?

If I understand it right, it can not collide. args argument directive is an array of strings, so the keys are incremental integers, but query arguments have string keys. After merging all entries should persist, I think...

@faiverson
Copy link
Contributor Author

Discussion aside, should we have the argument args? So far I can see it is always an array of hardcoded strings. It doesn't have too much sense...
I didn't want to use the same name (I think is the best name) and override the current functionality. But I don't think args is very useful. I'd replace args with injectArgs

@spawnia
Copy link
Collaborator

spawnia commented Nov 13, 2019

I am not planning to make a new major version with breaking changes soon.

Adding static values can be useful. The directive definition does lie a bit, actually you can pass a map as well, e.g.: @can(args: { foo: "bar" }). That means there can be conflicts, we have to define which is preferred.

I think it makes sense to prefer the static values, since that makes it more predictable that those are always passed and allows overwriting client given values for certain scenarios. Doing it the other way around is basically the same as setting default values, which is already possible in the argument definition itself.

@faiverson
Copy link
Contributor Author

I am not planning to make a new major version with breaking changes soon.

Adding static values can be useful. The directive definition does lie a bit, actually you can pass a map as well, e.g.: @can(args: { foo: "bar" }). That means there can be conflicts, we have to define which is preferred.

I think it makes sense to prefer the static values, since that makes it more predictable that those are always passed and allows overwriting client given values for certain scenarios. Doing it the other way around is basically the same as setting default values, which is already possible in the argument definition itself.

Ok, so we need to keep the args as it is right now.
I made the new test when we have both args and injectArgs.

@spawnia
Copy link
Collaborator

spawnia commented Nov 13, 2019

I did not realize that it is possible to just pass multiple arguments to the checks. That means we do not have to merge, nice.

@faiverson had a pass over the PR, let me know if the changes are fine.

@spawnia spawnia changed the title inject input variables in policy through can directive Add injectArgs option to @can directive to pass along client defined arguments to the policy check Nov 14, 2019
@spawnia spawnia merged commit 09a0df9 into nuwave:master Nov 14, 2019
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.

Pass arguments fields to policy in @can directive
4 participants