[GitHub] [geode] jujoramos opened pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["jujoramos (GitHub)" <gi...@apache.org>]

Added distributed tests to verify the required constraints are met
whenever the MethodInvocationAuthorizer is changed in runtime.

- Once the authorizer is changed, all queries executed afterwards
  should use the newly configured authorizer.
- Once a query execution starts, the authorizer used can not be
  changed for that particular query.
- Continuous queries already running should pick up the new authorizer
  the next time the query is internally executed to detect whether an
  event matches or not. If the CQ has methods not allowed by the newly
  configured authorizer, any matching events from that moment on should
  invoke 'onError' instead of 'onEvent' on the associated 'CqListener'.
- Indexes should pick up the newly configured authorizer the next time
  an entry is added or removed from the index, and the index itself
  should be marked as invalid if it uses method invocations not allowed
  by the newly configured authorizer.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["DonalEvans (GitHub)" <gi...@apache.org>]

Here (and in other tests) could the "getName" (or "getId" if that's the method used) string be extracted to a variable so that we can avoid having the string hardcoded in two different places?

[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["DonalEvans (GitHub)" <gi...@apache.org>]

Instead of two method calls here, it might be more efficient to assign the `internalCache` variable first and then assert that the variable is not null. This applies to almost every time `ClusterStartupRule` is used to retrieve a cache in this file and the other file in this PR.

[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["DonalEvans (GitHub)" <gi...@apache.org>]

Typo here, should be "it is not affected by".

[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["DonalEvans (GitHub)" <gi...@apache.org>]

Here and in other tests, could we extract the index name "NameIndex" (or "IdIndex" in tests where that's used) to a variable, just to avoid having it hardcoded in two different places?

[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["DonalEvans (GitHub)" <gi...@apache.org>]

Could this be reworded to be a little clearer? Maybe something like "changes the {@link MethodInvocationAuthorizer} to a custom one that allows the method that is invoked by the index and executes"

[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["jujoramos (GitHub)" <gi...@apache.org>]

Done!.

[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["DonalEvans (GitHub)" <gi...@apache.org>]

Would it be possible to have this `QueryObject` be a class field? The values passed to its constructor seem to be arbitrary so it should be fine for them to be the same in every test case, and it would avoid having to create a new one for every test case.

[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4416: GEODE-7351: OQL Method Authorizer Constraints

["jujoramos (GitHub)" <gi...@apache.org>]

Done!

[ Full content available at: https://github.com/apache/geode/pull/4416 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org