You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by GitBox <gi...@apache.org> on 2020/02/22 07:56:25 UTC
[GitHub] [unomi] RonBarabash opened a new pull request #131: Add the ability
to update event by item id
RonBarabash opened a new pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131
Hey,
As we understand from the Unomi docs, events are an immutable list of objects the can only be appended too.
As more and more event driven system are being based on tools such as Kafka, we want to have the ability to guard ourselves from receiving duplicate events as consumers usually operates in "at least once" semantics.
We thought that having the ability to pass in the itemId as a parameter to the context servlet would help us in achieving that goal without breaking the current behaviour.
We also want to add the ability to ignore duplicate events in some rules we define, hence we added the `raiseEventOnlyOnce` parameter to the rule creation.
I know that this is modifying `Event`, which is a Unomi core entity.
We were wondering if this is something u might want to consider adding to the core functionality.
If not, is there a way for us to achieve such behaviour only by extending or creating a plugin for Unomi?
Your help and thoughts would be kindly appreciated,
Cheers,
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386829687
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
hey @sergehuber,
I understand, i added your suggestion to the pr,
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-591936535
@jbonofre i added the check for session id as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-590799967
Hey all,
Firstly, Thank you for your responses.
Our use case with Unomi is as follows:
We want to build a user profile based on Internal System events that are transported through Kafka, for example When a user submits a new purchase, an event is being fired to the "OrderCreated/Updated" topic with the purchase details (Email, OrderAmount etc.).
At some point in time let's say we want to add a new property to the purchase details and send it to all past events as well (for instance: OrderCurrency). We have the ability to stream all of the events from beginning time and the relevant consumers should be able to update past events with the new property. This scenario is something we already encountered several times and we build our systems in a way that entities can be updated at their persistent target.
Again, if there is a way we can achieve this by not changing the core Unomi entities i would be more than happy to hear.
In the way we architect our system, Unomi is not accessible from the outside (i.e the internet) of our systems, hence we do not have any security considerations.
@sergehuber What do u mean by secondary id? Should we add a new property to Event and that would act as our id for updating?
@sigdestad I agree with you, Can u elaborate on how would u implement this without breaking event immutability?
Looking forward for your responses,
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-613001796
Hello Ron,
Thanks for all this, I'll look at it asap.
Regards,
Serge...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-612940060
Hey @sergehuber & @jbonofre,
I added some itests, can u please take a look and see if this is what u had in mind? if there are more use cases u wish to test let me know and ill add them...
Testing this stuff is quite tiresome as most of the underlying processes are async which forces the tests to wait for them.
anyhow, if there is another place u wish to document this let me know,
Much appreciated,
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r385715525
##########
File path: api/src/main/java/org/apache/unomi/api/rules/Rule.java
##########
@@ -132,6 +134,15 @@ public boolean isRaiseEventOnlyOnceForProfile() {
return raiseEventOnlyOnceForProfile;
}
+ /**
+ * Determines whether the event raised when the rule is triggered should only be raised once
+ *
+ * @return {@code true} if the rule-triggered event should only be raised once per profile
+ */
+ public boolean isRaiseEventOnlyOnce() {
+ return raiseEventOnlyOnce;
+ }
+
Review comment:
A setter is missing, this probably won't work well with JSON deserialization otherwise.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386956917
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
Im sorry @sergehuber but i dont understand why creating a new item might solve this.
Its also very unscalable to create a new item per event type we want to add to the system.
Do u think maybe we should talk in over the phone for u to get a better understanding on why we need this feature?
And again, Why not add a generic behaviour and functionality that should work for some of the use cases with Unomi? this PR does not suggesting breaking changes or a change in concept. having the ability to update events to me seems trivial, some of the users of Unomi can enable it, and some dont, even more so when we guarded this using the 3rd party id check...
Creating Item Types for each Event Type for me is abusing the system to do something that it doesn't suppose to do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386832109
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
Also, if i missed something ion the conversation threads let me know :)
Thank you for your responsiveness,
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r409491322
##########
File path: manual/src/main/asciidoc/updating-events.adoc
##########
@@ -0,0 +1,85 @@
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+=== Updating Events Using the Context Servlet
Review comment:
Looks good, but there is just a reference missing from the doc index, otherwise this document will not show up in the manual. Do you know where to add it ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r383095642
##########
File path: services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java
##########
@@ -250,6 +250,11 @@ private void getEventProperties(Map<String, Map<String, Object>> mappings, List<
}
}
+ public boolean hasEventAlreadyBeenRaised(Event event) {
+ Event pastEvent = this.persistenceService.load(event.getItemId(), Event.class);
Review comment:
Actually the hasEvent* methods are a known performance issue because they perform searches over events which can be problematic for large event sets. However, this implementation seems better since we are only searching for a single event id.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386869013
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
Hmm... I'm sorry to bring this up now but it seems like this is not a proper usage of events. Events are just what they are: objects that indicate something has happened at a certain time.
What you are describing in the use case seems more like you would benefit more from adding a new Item type called "Review" and then using events to update the Review items. This could be easily done just by building your own plugin. You could also build your own custom API to do any queries or aggregations you need on your custom item type.
I know this might come as a (big?) change, but with your use case it makes things a little different.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber edited a comment on issue #131: Add the ability
to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber edited a comment on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-613003228
I just had a quick look at the code and it looks good, I'll want to test it before merging it.
We also talk about documenting the feature and explaining it, will you contribute this separately (you could for example add content to the manual) ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r383096239
##########
File path: api/src/main/java/org/apache/unomi/api/Event.java
##########
@@ -82,47 +83,83 @@ public Event() {
* @param target the target of the event if any
* @param timestamp the timestamp associated with the event if provided
*/
- public Event(String eventType, Session session, Profile profile, String scope, Item source, Item target, Date timestamp) {
- super(UUID.randomUUID().toString());
- this.eventType = eventType;
- this.profile = profile;
- this.session = session;
- this.profileId = profile.getItemId();
- this.scope = scope;
- this.source = source;
- this.target = target;
-
- if (session != null) {
- this.sessionId = session.getItemId();
- }
- this.timeStamp = timestamp;
+ public Event(String itemId, String eventType, Session session, Profile profile, String scope, Item source, Item target, Date timestamp) {
+ super(itemId);
Review comment:
The whole idea of using server-generated IDs for events is that Unomi can process requests coming directly from the Internet and therefore they cannot be trusted. Allowed IDs to be client specified can potentially open the system to attacks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386861136
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
Let me know if there is anything i can do to help u test the migration rules.
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386956917
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
Im sorry @sergehuber but i dont understand why creating a new item might solve this.
Its also very unscalable to create a new item per event type we want to add to the system.
Do u think maybe we should talk in over the phone for u to get a better understanding on why we need this feature?
And again, Why not add a generic behaviour and functionality that should work for some of the use cases with Unomi? this PR does not suggesting breaking changes or a change in concept. having the ability to update events to me seems trivial, some of the users of Unomi can enable it, and some dont.
Creating Item Types for each Event Type for me is abusing the system to do something that it doesn't suppose to do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386846792
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
Hi Ron, thanks for adding the suggestion, this is much better. Now all that is left is to check for migration issues but I have to test the branch to check for that.
Actually now that you mention it you didn't give me any feedback on this part of my suggestion: "Another "more Unomi compliant" would be to, instead of replying the SAME events, you would send a new event of type "updateEvent" (or any name you choose), that would have an associated rule that would modify existing events. The disadvantage of this method is that events might grow quickly every type you update the events, but at least you would have full traceability of what happened overtime."
Finally, I'd love to have more context on what you are using this for, because I wonder for example how often you see these model changes and if you could give me more examples of real cases? I'm just curious here because I haven't encountered this much for the moment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-590175439
Hello Ron,
Could you explain a little bit more why you need to update events ? Maybe an example could be great?
My biggest concern with this PR is that it introduces the potential for security vulnerabilities. As secured events such as logins or profile properties events exist in the system, re-using itemIds could make it possible to overwrite or retrieve profile data.
Maybe an secondary ID could be used on the events to do what you need ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sigdestad commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
sigdestad commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-590184178
Imho this would break a fundamental concept in Unomi of immutable events. Ability to modify an event will break any possibility to playback a chain of events and any action resulting from the event. Also, It might lead to inconsistence in terms of other data. If you need to "change" an event, you should rather register an event that undoes the initial.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-590177604
Or another solution might be to allow this operation only for authorized third party servers by using something like the EventService.isEventAllowed mechanism ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r385716835
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -67,8 +67,12 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
if (events != null && !(profile instanceof Persona)) {
for (Event event : events) {
if (event.getEventType() != null) {
- Event eventToSend = new Event(event.getEventType(), session, profile, event.getScope(), event.getSource(),
- event.getTarget(), event.getProperties(), timestamp, event.isPersistent());
+ Event eventToSend;
+ if (event.getItemId() != null) {
+ eventToSend = new Event(event.getItemId(), event.getEventType(), session, profile, event.getScope(), event.getSource(), event.getTarget(), event.getProperties(), timestamp, event.isPersistent());
Review comment:
This is the most dangerous part of this modification, as it allows an itemId to come from a REST request. This should be secured after the eventService.isEventAllowed part and this last one should only be allowed if a Unomi-Key is sent and only allowed from configured IP addresses. See the relevant part in the documentation: http://unomi.apache.org/manual/latest/index.html#_secured_events_configuration
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-613424998
Hey @sergehuber,
No problems I'll add it to the manual in this pr,
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-614465638
Hey @sergehuber,
Added a new doc to the manual, let me know what u think,
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-613003228
I just had a quick look at the code and it looks good, I'll want to test it before merging it.
We also talk about documenting the feature and explaining it, will you contribute this separately (you could for example add content to the manual).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r392120875
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
Following our meeting it was decided to:
- Move the event update logic into a management API (tracked using this ticket : https://issues.apache.org/jira/projects/UNOMI/issues/UNOMI-288?filter=allopenissues)
- Update the event collector to reject an event with an existing ID. This also means that the logic inside the rule service probably isn't needed anymore?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash edited a comment on issue #131: Add the ability
to update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash edited a comment on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-593095668
Hey @sergehuber,
I updated the code to use the `isEventAllowed` mechanism before creating an event with a given item id...
Let me know if there is anything else i need to in order to get this pr in,
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-593095668
Hey @sergehuber,
I update the code to use the `isEventAllowed` mechanism before creating an event with a given item id...
Let me know if there is anything else i need to in order to get this pr in,
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sigdestad commented on issue #131: Add the ability to
update event by item id
Posted by GitBox <gi...@apache.org>.
sigdestad commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-590185855
Also - are you saying repeated events is cause by a technichal problem in your event streaming pipeline, or something else? If you only want to avoid duplicate entries, this can be handled in other ways than modifying existing events.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] jbonofre commented on issue #131: Add the ability to update
event by item id
Posted by GitBox <gi...@apache.org>.
jbonofre commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-591039028
@lyogev yeah, I think it's fair enough.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] lyogev commented on issue #131: Add the ability to update
event by item id
Posted by GitBox <gi...@apache.org>.
lyogev commented on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-591034828
Regarding security we can maybe make sure we are at least in the same session, @sergehuber will that suffice?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r383096052
##########
File path: persistence-elasticsearch/core/src/main/resources/META-INF/cxs/mappings/rule.json
##########
@@ -42,6 +42,9 @@
},
"raiseEventOnlyOnceForSession": {
"type": "boolean"
+ },
Review comment:
I'm not sure how this modification would impact existing rule sets. Could you test this before and after adding this definition? If it fails this means a migration using the Migration commands will need to be implemented for this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r410161345
##########
File path: manual/src/main/asciidoc/updating-events.adoc
##########
@@ -0,0 +1,85 @@
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+=== Updating Events Using the Context Servlet
Review comment:
Hey @sergehuber added a reference to the index.adoc file,
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r385714960
##########
File path: api/src/main/java/org/apache/unomi/api/Event.java
##########
@@ -82,47 +83,83 @@ public Event() {
* @param target the target of the event if any
* @param timestamp the timestamp associated with the event if provided
*/
- public Event(String eventType, Session session, Profile profile, String scope, Item source, Item target, Date timestamp) {
- super(UUID.randomUUID().toString());
- this.eventType = eventType;
- this.profile = profile;
- this.session = session;
- this.profileId = profile.getItemId();
- this.scope = scope;
- this.source = source;
- this.target = target;
-
- if (session != null) {
- this.sessionId = session.getItemId();
- }
- this.timeStamp = timestamp;
+ public Event(String itemId, String eventType, Session session, Profile profile, String scope, Item source, Item target, Date timestamp) {
+ super(itemId);
Review comment:
Ok so after reflection this change should be ok.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber edited a comment on issue #131: Add the ability
to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber edited a comment on issue #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#issuecomment-590175439
Hello Ron,
Thank you so much for your contribution, it is very appreciated. However, as a reviewer there are some points I would like to review. Thanks for your understanding.
Could you explain a little bit more why you need to update events ? Maybe an example could be great?
My biggest concern with this PR is that it introduces the potential for security vulnerabilities. As secured events such as logins or profile properties events exist in the system, re-using itemIds could make it possible to overwrite or retrieve profile data.
Maybe an secondary ID could be used on the events to do what you need ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] RonBarabash commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
RonBarabash commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386860870
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
Hey @sergehuber,
So the solution u suggested might work but it doesn't seem very scalable to create a new event type for each entity, i think it would create a lot of mess within the data, and how would you treat these events on the query side, if you would want to create a rather complex aggregation which involves the update events + the created events? i would say that it would be over-complicating things for a rather simple & straight solution.
I think that if you would like to have full traceability of what happened overtime u might want to save this somewhere else outside of Unomi, maybe fire state changed events to Kafka and take it from there like ([CDC](https://en.wikipedia.org/wiki/Change_data_capture))
About our use case:
So we are now at the beginning of building Yotpo's internal CDP based on apache Unomi.
As i mentioned earlier. we compose the user profiles based on internal system events which are themselves state changed events of core Yotpo entities. (i.e OrderCreated, ReviewCreated, PointsRedeemed etc)
All of these distributed systems are sending events to Kafka, from there we trigger Unomi Consumers which transform and validate the events properties and store them in Unomi.
We work in a matter that event publishers are consumer agnostic, meaning Unomi is just another downstream consumer of events.
We have scenarios of which entities are being created and after a while being updated. for example: a customer left a review which triggered ReviewCreate event. for each review we are performing sentiment analysis asynchronously (after the review was created, we send the text through an NLP pipeline) which eventually update the review entity with a sentiment score and triggers the ReviewUpdated event.
We need to be able to store the sentiment score within Unomi, and associate it with the relevant event that was already registered in Unomi.
We can dig deeper to some of the other use cases if u would like offline :)
nonetheless, the change proposed in this PR is supporting our use case without breaking the default use cases you guys had in mind while building Unomi.
10x
Ron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [unomi] sergehuber commented on a change in pull request #131: Add
the ability to update event by item id
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #131: Add the ability to update event by item id
URL: https://github.com/apache/unomi/pull/131#discussion_r386430982
##########
File path: wab/src/main/java/org/apache/unomi/web/ServletCommon.java
##########
@@ -73,6 +73,9 @@ public static Changes handleEvents(List<Event> events, Session session, Profile
logger.warn("Event is not allowed : {}", event.getEventType());
continue;
}
+ if (event.getItemId() != null) {
Review comment:
I think it would be best to check if the thirdPartyId != null in that condition so it would become:
if (thirdPartyId != null && event.getItemId() != null) {
This way the itemId would only be accepted if a configured server Id is doing the request and it provides the Unomi Key.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services