Added lenientValidation support to REMReM generate #128

raja-maragani · 2020-07-17T14:16:41Z

Applicable Issues

eiffel-community/eiffel-remrem-generate#171

Description of the Change

Added Lenient Validation support to REMReM Semantics protocol, The Lenient Validation will perform the only mandatory field validation and non-mandatory validation failures will place in Eiffel message as a new property(remremGenerateFailures)

Alternate Designs

Benefits

The REMReM generate validation will not fail on non-fetal Eiffel message field issues. This feature is an option to the user, the user can decide the enable/disable the Lenient Validation. By default it is disabled.

Possible Drawbacks

Sign-off

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

Signed-off-by: @raja-maragani

magnusbaeck

The PR doesn't reference any issue or anything else that explains why this change is being made. It's not clear to me why it would be useful to only enable validation on mandatory fields (because that's what lenient validation means, right?). Why would we want to expose a feature that makes it easy for users to produce invalid messages?
The documentation should be updated to explain the lenient validation feature and exactly what it means.

src/test/java/com/ericsson/eiffel/remrem/semantics/ServiceTest.java

src/main/java/com/ericsson/eiffel/remrem/semantics/SemanticsService.java

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java

pom.xml

CHANGELOG.md

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java

raja-maragani · 2020-07-20T07:17:17Z

The PR doesn't reference any issue or anything else that explains why this change is being made. It's not clear to me why it would be useful to only enable validation on mandatory fields (because that's what lenient validation means, right?). Why would we want to expose a feature that makes it easy for users to produce invalid messages?

The documentation should be updated to explain the lenient validation feature and exactly what it means.

Created an issue for this change: eiffel-community/eiffel-remrem-generate#171
I will update the wiki also once PR's approved on this feature.

raja-maragani · 2020-07-20T10:23:14Z

@magnusbaeck added code changes for your comments.

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java

raja-maragani · 2020-07-23T09:15:59Z

@magnusbaeck added code changes for your comments.

@magnusbaeck do you have time to review this PR?

magnusbaeck · 2020-07-23T20:08:30Z

I was waiting for a response to my questions in eiffel-community/eiffel-remrem-generate#171, but I've been away for the past couple of days so I haven't been able to digest what you wrote.

magnusbaeck · 2020-07-26T20:42:34Z

I've posted a comment in the issue.

I will update the wiki also once PR's approved on this feature.

The wiki contents is stored in the wiki subdirectory in the repository, so why not update the documentation in this PR?

raja-maragani · 2020-07-27T08:49:55Z

I've posted a comment in the issue.

I will update the wiki also once PR's approved on this feature.

The wiki contents is stored in the wiki subdirectory in the repository, so why not update the documentation in this PR?

Hi, later I identified the documentation part.
I updated documentation also.
Documentation changes are available in the below links.
https://github.com/raja-maragani/eiffel-remrem-generate/blob/master/wiki/markdown/usage/generate-cli.md
https://github.com/eiffel-community/eiffel-remrem-generate/blob/509510486c00c7c1d607af13da66fb6e38928381/wiki/markdown/usage/lenientvalidation.md

Umadevi-Kapu · 2020-07-30T08:09:32Z

+1

raja-maragani · 2020-08-09T16:46:47Z

I got 2 approvals.
@magnusbaeck I hope requested changes/clarifications are done, could you please check once.
Thanks in advance.

magnusbaeck · 2020-08-09T20:10:51Z

I hope requested changes/clarifications are done, could you please check once.

I'll have a look during Monday.

magnusbaeck · 2020-08-10T21:21:48Z

src/main/java/com/ericsson/eiffel/remrem/semantics/SemanticsService.java

+    }
+
+    @Override
+    public String generateMsg(String msgType, JsonObject bodyJson, Boolean lenientValidation) {


Didn't we agree to not call this lenientValidation?

from our latest discussion we are using two param names i.e
Config param - lenientValidationEnabledToUsers
Rest Param - okToLeaveOutInvalidOptionalFields
Naming still we are using the lenient validation

magnusbaeck · 2020-08-10T21:31:24Z

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java

            log.debug(message, e);
            throw new EiffelValidationException(message, e);
        }
    }
+    private void handleErrorReport(JsonObject jsonObjectInput, ProcessingReport report) throws EiffelValidationException {


Blank line between methods please (here and in several other places in the file).

magnusbaeck · 2020-08-10T21:41:35Z

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java

+        if (!report.isSuccess()) {
+            log.error("Eiffel message validation failed");
+            log.error(jsonObjectInput.toString());
+            throw new EiffelValidationException(getErrorsList(report));


The strategy for error logging and exception handling seems haphazard. Logging and throwing an exception from the same place usually isn't a great idea (you're likely to log duplicate information, plus it's makes the code verbose). Here you're logging the input message at the error level with no information about the error itself, throwing an exception with the errors, which is caught on line 113 and logged at the debug level, then re-raised as a new exception that wraps the original exception. Perhaps the exception from line 116 is also logged somewhere?

Wouldn't it be better to raise an exception that contains all relevant information and log that in one place?

But I realize most of this is old code that shouldn't be modified in this PR.

Also, when logging, please don't call the logger multiple times for what's effectively a single message (like the case with lines 121-122).

Yes, I agree loggers are not required here, we are raising the exception with validation errors.

magnusbaeck · 2020-08-10T21:48:41Z

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java

+    private static final String SLASH = "/";
+    private static final String DOT = ".";
+    private static final String REMREM_GENERATE_FAILURES = "remremGenerateFailures";
+    private static final String PATH2 = "path";


Why PATH2 and not PATH?

magnusbaeck · 2020-08-10T21:52:05Z

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java

+        object.addProperty(PATH2, path);
+        return object;
+    }
+    private JsonObject addRemremGenerateFailuresToCustomData(JsonObject inputJson, JsonArray remremGenerateFailures) {


Since you're mutating the input JsonObject, why not make this a void method? Returning a JsonObject reference gives the impression that the input JsonObject is left untouched.

Yes but I directly return the output and inputJson is formed while calling this method.

magnusbaeck · 2020-08-10T21:55:31Z

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java

+    private String getPath(String errorPath) {
+        final Pattern pattern = Pattern.compile(REGEX_PATH, Pattern.MULTILINE);
+        final Matcher matcher = pattern.matcher(errorPath);
+        while (matcher.find()) {
+            if(matcher.group(0).length()>1) {
+                errorPath = errorPath.substring(0, errorPath.indexOf(matcher.group(0))+matcher.group(0).length());
+                errorPath = errorPath.replaceAll(matcher.group(0), "["+matcher.group(0).substring(1)+"]");
+            }
+        }
+        return "$" + errorPath.replace(SLASH, DOT);
+    }


No tests for this code?

Its private method and we don't have the unit tests for private methods. but we added unit tests for validations with lenient validations.

raja-maragani · 2020-08-13T16:05:02Z

If no other comments, I will merge this PR.

Added lenientValidation support to REMReM generate

8314a76

raja-maragani requested review from e-backmark-ericsson, SantoshNC68, Umadevi-Kapu and a team July 17, 2020 14:18

magnusbaeck requested changes Jul 19, 2020

View reviewed changes

magnusbaeck mentioned this pull request Jul 19, 2020

Added lenientValidation support to REMReM generate eiffel-community/eiffel-remrem-generate#170

Merged

resolved review comments and version uplifted(Minor version uplift)

56f8c66

Umadevi-Kapu reviewed Jul 20, 2020

View reviewed changes

src/main/java/com/ericsson/eiffel/remrem/semantics/validator/EiffelValidator.java Outdated Show resolved Hide resolved

spell mistake corrected.

25010b1

raja-maragani requested review from Divya-Kuppula and Umadevi-Kapu July 21, 2020 11:31

removed type validation from lenient.

858712e

raja-maragani requested review from durga-vasaadi, Navyatha-reddi and a team August 7, 2020 05:34

SantoshNC68 approved these changes Aug 7, 2020

View reviewed changes

magnusbaeck requested changes Aug 10, 2020

View reviewed changes

commit for code review comments

e431767

raja-maragani merged commit 07d8ab4 into eiffel-community:master Aug 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added lenientValidation support to REMReM generate #128

Added lenientValidation support to REMReM generate #128

raja-maragani commented Jul 17, 2020 •

edited

Loading

magnusbaeck left a comment

raja-maragani commented Jul 20, 2020

raja-maragani commented Jul 20, 2020

raja-maragani commented Jul 23, 2020

magnusbaeck commented Jul 23, 2020

magnusbaeck commented Jul 26, 2020

raja-maragani commented Jul 27, 2020

Umadevi-Kapu commented Jul 30, 2020

raja-maragani commented Aug 9, 2020

magnusbaeck commented Aug 9, 2020

magnusbaeck Aug 10, 2020

raja-maragani Aug 13, 2020

magnusbaeck Aug 10, 2020

raja-maragani Aug 13, 2020

magnusbaeck Aug 10, 2020

raja-maragani Aug 13, 2020

magnusbaeck Aug 10, 2020

raja-maragani Aug 13, 2020

magnusbaeck Aug 10, 2020

raja-maragani Aug 13, 2020

magnusbaeck Aug 10, 2020

raja-maragani Aug 13, 2020

raja-maragani commented Aug 13, 2020

Added lenientValidation support to REMReM generate #128

Added lenientValidation support to REMReM generate #128

Conversation

raja-maragani commented Jul 17, 2020 • edited Loading

Applicable Issues

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Sign-off

magnusbaeck left a comment

Choose a reason for hiding this comment

raja-maragani commented Jul 20, 2020

raja-maragani commented Jul 20, 2020

raja-maragani commented Jul 23, 2020

magnusbaeck commented Jul 23, 2020

magnusbaeck commented Jul 26, 2020

raja-maragani commented Jul 27, 2020

Umadevi-Kapu commented Jul 30, 2020

raja-maragani commented Aug 9, 2020

magnusbaeck commented Aug 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raja-maragani commented Aug 13, 2020

raja-maragani commented Jul 17, 2020 •

edited

Loading