[GitHub] nifi-registry issue #133: NIFIREG-190
Github user jdye64 commented on the issue: https://github.com/apache/nifi-registry/pull/133 Looks like the Travis CI build is failing trying to extract the npm binary for building the web ui section. This doesn't have anything to do with the PR so don't believe the build is truly failing here... ---
[GitHub] nifi-registry issue #133: NIFIREG-190
Github user pvillard31 commented on the issue: https://github.com/apache/nifi-registry/pull/133 Had a quick look through the code and it LGTM, will try to give it a try today or tomorrow if no else has the cycles to do so. ---
[GitHub] nifi-registry issue #133: NIFIREG-190
Github user kevdoran commented on the issue: https://github.com/apache/nifi-registry/pull/133 Will review... ---
[GitHub] nifi-registry issue #133: NIFIREG-190
Github user jdye64 commented on the issue: https://github.com/apache/nifi-registry/pull/133 I had contemplated placing the logic that checked if the hook provider should be invoked for an event or not right above https://github.com/apache/nifi-registry/blob/7e4e9713406f3bb3ac1544b6ad69fbdd437f794c/nifi-registry-framework/src/main/java/org/apache/nifi/registry/event/EventService.java#L73 but decided to place that logic in each hook provider in case someone wanted to write their own custom hook provider and want to apply different white listing logic that would allow them to circumvent such filtering. How do you both feel about that? ---
[GitHub] nifi-registry issue #133: NIFIREG-190
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/133
If we are allowing each implementation to decide the logic in the handle
method, I don't see the value in the "share property" concept as documented:
> There are certain properties that are shared amongst all of the NiFi
Registry provided Event Hook implementations.
I don't think we should introduce the concept of a shared property that is
documented as being honored by all implementations when we really have no way
to guarantee that.
In this PR, the implementation that does handle the Event Whitelist is
implemented as an Abstract base class in the framework, which would make it
difficult for third-party extension providers to implement (or they could
ignore it if they want as it is not part of the API).
If we want to make it universal, I think we would need to introduce a
dedicated field for whitelist that is enforced by the framework code (perhaps
using a decorator / wrapper), not the extension. Otherwise, don't think we make
it universal at all, it can just be an optional thing that third parties can
choose to implement.
One way to make it more obvious to custom implementations that this is
intended to be implemented would be to make it part of the interface they have
to implement (otherwise it is just documentation and hard to enforce). For
example, we could add a new method with a default implementation to the
`EventHookProvider` interface. For example:
```
default boolean shouldHandle(EventType eventType) {
return true;
}
```
If it's not overridden with a custom implementation, the event hook
provider will always be called. If it is overridden, they can return a boolean
for the event types they are interested in, either as a hard-coded set or a
user-configurable set (they would still need to document their own
configuration property for that).
---
[GitHub] nifi-registry issue #133: NIFIREG-190
Github user jdye64 commented on the issue: https://github.com/apache/nifi-registry/pull/133 Ok. After reading through all of these I'm thinking the best way to handle this and have it operate the way I had envisioned is to take the approach of adding the handleEvent() method in the EventProvider interface as outlined above. I'm going to start on that now and just remove the Abstract class all together. ---
[GitHub] nifi-registry issue #133: NIFIREG-190
Github user kevdoran commented on the issue: https://github.com/apache/nifi-registry/pull/133 Thanks @jdye64! That sounds good, I'll give it another look when those changes are ready. ---
[GitHub] nifi-registry issue #133: NIFIREG-190
Github user kevdoran commented on the issue: https://github.com/apache/nifi-registry/pull/133 A couple of other things I thought of: - The default [providers.xml](https://github.com/apache/nifi-registry/blob/7e4e9713406f3bb3ac1544b6ad69fbdd437f794c/nifi-registry-resources/src/main/resources/conf/providers.xml#L34) should maybe be updated to include placeholders/explanation for the whitelist properties. - By adding the `shouldHandle(EventType)` method to the EventHookProvider interface, should we move the conditional execution of the hook's `handle(Event)` method to a check where it is called in [EventService](https://github.com/apache/nifi-registry/blob/7e4e9713406f3bb3ac1544b6ad69fbdd437f794c/nifi-registry-framework/src/main/java/org/apache/nifi/registry/event/EventService.java#L73)? ---
[GitHub] nifi-registry issue #133: NIFIREG-190
Github user jdye64 commented on the issue: https://github.com/apache/nifi-registry/pull/133 I think if the EventType is null, which I'm like you I don't know if it ever could be?? Then the whitelisting logic should be ignored and the Event Handlers should all be invoked. That makes the most sense to me. ---
