Hi Felix, Thanks for looking at it.
The patch in [0] enforces a 1:1 mapping from the methods of the TenantManager and the events being sent. For example, if a user invoke TenantManager#setProperty, the related event "org/apache/sling/tenant/properties/SET" is being sent. AFAIU your proposal is about extending the scope of the information being sent by differentiating between properties being "updated" or being "added" for the first time. I think it makes sense since there is no clean way for the receiver of the event to figures this information out. Assuming we enforce this differentiation, then the invocation of a single API method (e.g. TenantManager#setProperties) may trigger events that require to carry both "updated" and "added" properties. Therefore, grouping the properties in the same event is kind of required. Regarding extending this pattern to pack the removed properties in the same event, it may make sense as well. However we would miss a level of granularity that is available in the patch (allows to register to a subset of events). Arguably, the receiver of the event could fairly easily filter the types of properties he does not need (thus achieving virtually the same). The difference would be that the EventAdmin may dispatch unused events in the one-event-to-fit-them-all approach. Regarding passing the values in the event, the advantage I see is for the remove case, where the API user can't cleanly know what values have been removed. If knowing this information is a legit use case, then it make sense to me. Otherwise, I think passing only the parameter names is better for the security reason you noted and for performance reasons (we would not need to load and write all properties for each change and we would not need to send event containing potentially large objects). Regards, Timothee 2014-11-28 18:27 GMT+01:00 Felix Meschberger <[email protected]>: > I am looking at the patch attached to SLING-4207 [1] and wonder about the > property changes. > > Shouldn’t we just have a TENANT_UPDATED event with three additional > properties: > > * addedProperties > * removedProperties > * changedProperties > > I agree, that adding just the propertyNames makes sense to not leak > information. On the other hand, a Tenant can easily be retrieved and the > properties accessed, so leaking may not be that big of a problem. > > Plus: the added/removed/changed properties would not only be the > properties set on the respective initial call but also encompassing any > updates done by the TenantCustomizers. > > So, essentially the properties would be: > > addedProperties = newProperties without oldProperties; > removedProperties = oldProperties without newProperties; > changedProperties = newProperties and oldProperties; > > WDYT ? > > Regards > Felix > > [1] https://issues.apache.org/jira/browse/SLING-4207 > > > Am 28.11.2014 um 12:52 schrieb Felix Meschberger <[email protected]>: > > > > Hi > > > > This sounds useful to me. > > > > Lets do OSGi events to decouple the actual listener handling and event > distribution from the tenant manager. > > > > Regards > > Felix > > > > -- > > Typos caused by my iPhone > > > >> Am 27.11.2014 um 16:56 schrieb Timothée Maret <[email protected] > >: > >> > >> Hi, > >> > >> Currently, there is no clean way to detect when a tenant has been > >> added/removed/modified. > >> > >> We may detect when a change is required by implementing the > >> TenantCustomizer interface, however, it tells nothing about the actual > >> completion of the change. > >> We may listen for OSGI events under /etc/tenants but this requires the > user > >> to know where the tenants are located (which afaik, currently is not > >> exposed). > >> > >> In order to allow apps reacting on tenant changes, I propose to either: > >> > >> I. extend the current SPI with an interface TenantEventListener that > users > >> can implement ; or > >> II. send OSGI events form the current implementation. > >> > >> In both cases, the events should cover > >> * Tenant added > >> * Tenant removed > >> * Property added > >> * Property removed > >> > >> wdyt ? > >> > >> Regards, > >> > >> Timothee > > -- Timothée Maret
