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 2021/01/20 13:38:40 UTC
[GitHub] [unomi] giladw opened a new pull request #238: on segment creation, validate segment condition and logging errors
giladw opened a new pull request #238:
URL: https://github.com/apache/unomi/pull/238
----------------------------------------------------------------
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
[GitHub] [unomi] giladw commented on a change in pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
giladw commented on a change in pull request #238:
URL: https://github.com/apache/unomi/pull/238#discussion_r575822987
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
Review comment:
yes, i will go with the boolean option
----------------------------------------------------------------
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
[GitHub] [unomi] giladw commented on pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
giladw commented on pull request #238:
URL: https://github.com/apache/unomi/pull/238#issuecomment-766823415
https://issues.apache.org/jira/browse/UNOMI-419
----------------------------------------------------------------
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
[GitHub] [unomi] giladw commented on a change in pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
giladw commented on a change in pull request #238:
URL: https://github.com/apache/unomi/pull/238#discussion_r575823058
##########
File path: persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java
##########
@@ -305,6 +305,14 @@
*/
boolean testMatch(Condition query, Item item);
+ /**
+ * validates if a condition throws exception at query build.
+ *
+ * @param query the condition we're testing the specified item against
+ * @param item the item we're checking against the specified condition
+ * @return {@code true} if the item satisfies the condition, {@code false} otherwise
+ */
+ void validateCondition(Condition query, Item item);
Review comment:
thanks, changing to boolean
----------------------------------------------------------------
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
[GitHub] [unomi] giladw commented on pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
giladw commented on pull request #238:
URL: https://github.com/apache/unomi/pull/238#issuecomment-766823415
https://issues.apache.org/jira/browse/UNOMI-419
----------------------------------------------------------------
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
[GitHub] [unomi] dgriffon commented on a change in pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
dgriffon commented on a change in pull request #238:
URL: https://github.com/apache/unomi/pull/238#discussion_r570039357
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
+ if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
+ updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ }
+ // make sure we update the name and description metadata that might not match, so first we remove the entry from the map
+ persistenceService.save(segment, null, true);
+ updateExistingProfilesForSegment(segment);
+ } catch (Exception e) {
Review comment:
This should be done in the `validateCondition` function.
##########
File path: persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java
##########
@@ -305,6 +305,14 @@
*/
boolean testMatch(Condition query, Item item);
+ /**
+ * validates if a condition throws exception at query build.
+ *
+ * @param query the condition we're testing the specified item against
+ * @param item the item we're checking against the specified condition
+ * @return {@code true} if the item satisfies the condition, {@code false} otherwise
+ */
+ void validateCondition(Condition query, Item item);
Review comment:
I think this should return true or false (as the javadoc describes) or a check exception, it should not fail because of an unchecked exception.
As soon as we know that it can fail, I think it is better to handle it with a custom exception.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
Review comment:
if the `validateCondition` return a boolean, you could use it to throw a custom uncheck exception.
It is also possible to keep `validateCondition` with `void` but only throw an uncheck exception from it.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
+ if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
+ updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ }
+ // make sure we update the name and description metadata that might not match, so first we remove the entry from the map
+ persistenceService.save(segment, null, true);
+ updateExistingProfilesForSegment(segment);
+ } catch (Exception e) {
+ logger.error("failed at setSegmentDefinition, condition={}", segment.getCondition().toString(), e);
+ throw e;
Review comment:
Throwing an exception in here stops the execution, as the registration of a condition can be part of a process (like remove a segment), if it fails at that point, the rest of the execution won't be executed.
I think it could be better to not execute the code if the validation fail instead of throwing an exception.
----------------------------------------------------------------
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
[GitHub] [unomi] Taybou merged pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
Taybou merged pull request #238:
URL: https://github.com/apache/unomi/pull/238
----------------------------------------------------------------
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
[GitHub] [unomi] dgriffon commented on a change in pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
dgriffon commented on a change in pull request #238:
URL: https://github.com/apache/unomi/pull/238#discussion_r570039357
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
+ if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
+ updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ }
+ // make sure we update the name and description metadata that might not match, so first we remove the entry from the map
+ persistenceService.save(segment, null, true);
+ updateExistingProfilesForSegment(segment);
+ } catch (Exception e) {
Review comment:
This should be done in the `validateCondition` function.
##########
File path: persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java
##########
@@ -305,6 +305,14 @@
*/
boolean testMatch(Condition query, Item item);
+ /**
+ * validates if a condition throws exception at query build.
+ *
+ * @param query the condition we're testing the specified item against
+ * @param item the item we're checking against the specified condition
+ * @return {@code true} if the item satisfies the condition, {@code false} otherwise
+ */
+ void validateCondition(Condition query, Item item);
Review comment:
I think this should return true or false (as the javadoc describes) or a check exception, it should not fail because of an unchecked exception.
As soon as we know that it can fail, I think it is better to handle it with a custom exception.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
Review comment:
if the `validateCondition` return a boolean, you could use it to throw a custom uncheck exception.
It is also possible to keep `validateCondition` with `void` but only throw an uncheck exception from it.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
+ if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
+ updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ }
+ // make sure we update the name and description metadata that might not match, so first we remove the entry from the map
+ persistenceService.save(segment, null, true);
+ updateExistingProfilesForSegment(segment);
+ } catch (Exception e) {
+ logger.error("failed at setSegmentDefinition, condition={}", segment.getCondition().toString(), e);
+ throw e;
Review comment:
Throwing an exception in here stops the execution, as the registration of a condition can be part of a process (like remove a segment), if it fails at that point, the rest of the execution won't be executed.
I think it could be better to not execute the code if the validation fail instead of throwing an exception.
----------------------------------------------------------------
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
[GitHub] [unomi] giladw commented on a change in pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
giladw commented on a change in pull request #238:
URL: https://github.com/apache/unomi/pull/238#discussion_r575822822
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
+ if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
+ updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ }
+ // make sure we update the name and description metadata that might not match, so first we remove the entry from the map
+ persistenceService.save(segment, null, true);
+ updateExistingProfilesForSegment(segment);
+ } catch (Exception e) {
+ logger.error("failed at setSegmentDefinition, condition={}", segment.getCondition().toString(), e);
+ throw e;
Review comment:
I agree, im removing the try-catch, and throwing if validation fails
----------------------------------------------------------------
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
[GitHub] [unomi] giladw commented on a change in pull request #238: UNOMI-419 - on segment creation, validate segment condition and logging errors
Posted by GitBox <gi...@apache.org>.
giladw commented on a change in pull request #238:
URL: https://github.com/apache/unomi/pull/238#discussion_r575822932
##########
File path: services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
##########
@@ -233,14 +233,19 @@ public Segment getSegmentDefinition(String segmentId) {
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
- updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ try {
+ ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+ persistenceService.validateCondition(segment.getCondition(), new Profile());
+ if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
+ updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
+ }
+ // make sure we update the name and description metadata that might not match, so first we remove the entry from the map
+ persistenceService.save(segment, null, true);
+ updateExistingProfilesForSegment(segment);
+ } catch (Exception e) {
Review comment:
ok, moving the log to the validation.
----------------------------------------------------------------
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