Modified generate's endpoint generate to accept array of events #226

shudhansu-shekhar · 2024-06-19T10:24:52Z

Applicable Issues

Description of the Change

In this after making this change, generate service endpoint generate accept array of events or templates

Alternate Designs

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: [email protected]

z-sztrom · 2024-06-19T11:24:58Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+        } catch (JsonSyntaxException e) {
+            JsonObject errorResponse = new JsonObject();
+            log.error("Unexpected exception caught due to parse json data", e.getMessage());
+            String exceptionMessage = e.getMessage();


Use exceptionMessage at line 122, too.

z-sztrom · 2024-06-19T11:26:54Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                JsonObject jsonObject = bodyJson.getAsJsonObject();
+                JsonObject jsonObject1 = processEvent(jsonObject, msgProtocol, msgType, failIfMultipleFound, failIfNoneFound,
+                        lookupInExternalERs, lookupLimit, okToLeaveOutInvalidOptionalFields);
+                return new ResponseEntity<>(jsonObject1, HttpStatus.OK);


Use better name for jsonObjec1.

z-sztrom · 2024-06-19T11:28:41Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            } else {
+                errorResponse.addProperty("Status code", HttpStatus.BAD_REQUEST.value());
+                errorResponse.addProperty("result", "fail");
+                errorResponse.addProperty("message", "Invalid JSON format");


Add more hints: something like "either single template or array of templates is expected".

z-sztrom · 2024-06-19T11:29:50Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                return new ResponseEntity<>(jsonObject1, HttpStatus.OK);
+            } else {
+                errorResponse.addProperty("Status code", HttpStatus.BAD_REQUEST.value());
+                errorResponse.addProperty("result", "fail");


Line 125 states "Fail" with upper-case 'F'. Unify formatting.

z-sztrom · 2024-06-19T11:30:57Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                errorResponse.addProperty("message", "Invalid JSON format");
+                return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST);
+            }
+        } catch (Exception e) {


Don't use Exception, it's too general.

we already handled particular exception in helper method. This one is handled like this in original as well.

z-sztrom · 2024-06-19T11:36:02Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                parsedResponse = parser.parse(response);
+
+                if (lookupLimit <= 0) {
+                    eventResponse.addProperty("status code", HttpStatus.BAD_REQUEST.value());


"status code" with lower-case 's'. Upper-case 's' is used everywhere else. It's good idea to define constants for common strings.

make it similar everywhere

z-sztrom · 2024-06-19T11:42:57Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-                    return new ResponseEntity<>(parsedResponse, HttpStatus.BAD_REQUEST);
+                    eventResponse.addProperty("Status code", HttpStatus.BAD_REQUEST.value());
+                    eventResponse.addProperty("Result", "Fail");
+                    eventResponse.addProperty("Message", RemremGenerateServiceConstants.NOT_ACCEPTABLE);


Can we understand what went wrong reading the error message?

Added extra template error constant variable in RemremGenerateServiceConstants.class Now meesage is looking more understandable

z-sztrom · 2024-06-19T11:43:31Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-            } else {
-                return new ResponseEntity<>(parser.parse(RemremGenerateServiceConstants.NO_SERVICE_ERROR),
-                        HttpStatus.SERVICE_UNAVAILABLE);
+
            }
        } catch (REMGenerateException e1) {


z-sztrom · 2024-06-19T11:46:43Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

            }
+            eventResponse.addProperty("Result", "Fail");
+            eventResponse.add("Message", parser.parse(e1.getMessage()));


What if parsing fails?

z-sztrom · 2024-06-19T11:48:07Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            log.error("Unexpected error caught", e);
+            eventResponse.addProperty("Status code", HttpStatus.INTERNAL_SERVER_ERROR.value());
+            eventResponse.addProperty("Result", "Fail");
+            eventResponse.addProperty("Message", RemremGenerateServiceConstants.INTERNAL_SERVER_ERROR);


Why the failure reason is hidden from customer?

z-sztrom

Please, define constants for all properties like "status code", "message", etc.

z-sztrom · 2024-07-02T09:18:17Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-                if(lookupLimit <= 0) {
-                    return new ResponseEntity<>("LookupLimit must be greater than or equals to 1", HttpStatus.BAD_REQUEST);
+            if (lookupLimit <= 0) {
+                return new ResponseEntity<>("LookupLimit must be greater than or equals to 1", HttpStatus.valueOf(HttpStatus.BAD_REQUEST.value()));


This looks strange: HttpStatus.valueOf(HttpStatus.BAD_REQUEST.value()). Please, simplify.

z-sztrom · 2024-07-02T09:22:16Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                for (int i = 0; i < results.size(); i++) {
+                    JsonObject event = results.get(i).getAsJsonObject();
+                    if (event.has("meta")) {
+                        return new ResponseEntity<>(results, HttpStatus.OK);


Status code cannot be determined from the first element.

z-sztrom · 2024-07-02T09:23:23Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                return new ResponseEntity<>("LookupLimit must be greater than or equals to 1", HttpStatus.valueOf(HttpStatus.BAD_REQUEST.value()));
+            }
+
+            if (bodyJson.isJsonArray()) {


Check for null of bodyJson should be added.

z-sztrom · 2024-07-02T09:32:50Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-            else {
-                return new ResponseEntity<>(parser.parse(e1.getMessage()), HttpStatus.UNPROCESSABLE_ENTITY);
+        } catch (REMGenerateException e) {
+            if (e.getMessage().contains(Integer.toString(HttpStatus.NOT_ACCEPTABLE.value()))) {


Dirty handling of exception. Each of the 'if's body has the same body, only some hard-coded values differ. Please, redesign handling.

sorry but inside every if condition is doing different job and it seems there is some confusion here, it seems you provide comment on older one?

z-sztrom · 2024-07-02T09:36:03Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                String response = msgService.generateMsg(msgType, event, isLenientEnabled(okToLeaveOutInvalidOptionalFields));
+                parsedResponse = parser.parse(response);
+
+                if (!parsedResponse.getAsJsonObject().has(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD)) {


Don't call the same method twice on the same target. Please, optimize.

yeah but where i can't find like this where you suggest?

z-sztrom · 2024-07-02T09:36:30Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+
+            if (msgService != null) {
+                String response = msgService.generateMsg(msgType, event, isLenientEnabled(okToLeaveOutInvalidOptionalFields));
+                parsedResponse = parser.parse(response);


Could parsedReseponse remain null?

but how if there is response then how parsedResponse will be null?

z-sztrom · 2024-07-02T09:37:21Z

.../test/java/com/ericsson/eiffel/remrem/generate/integrationtest/EiffelRemremControllerIT.java

@@ -173,7 +173,7 @@ public void testDuplicateKeyInBody() throws IOException {
                .contentType("application/json")
                .body(activityFinishedDuplicateKeysBody)
                .when()
-                    .post("/eiffelsemantics?msgType=EiffelActivityFinishedEvent")
+                    .post("/eiffelsemantics?msgType=EiffelActivityFinishedEven")


Why 't' has been removed?

yeah sorry i just testing with this....i will do this

z-sztrom · 2024-07-02T09:46:29Z

...rc/test/java/com/ericsson/eiffel/remrem/generate/service/EiffelRemremControllerUnitTest.java

        assertEquals(elem.getStatusCode(), HttpStatus.BAD_REQUEST);
    }

+    @Test
+    public void testEiffel3FailureEventArray() throws Exception {


Does the test pass? Real events are passed to generate method instead of templates...

yes....but we can use this real event?

z-sztrom

Please, check log messages. Unify them, ensure they have separators in front of appended values (e.g. "...URL" + url), etc.

z-sztrom · 2024-07-15T10:43:23Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+        }
+    }
+
+    public ResponseEntity<JsonObject> createResponseEntity(String statusCode, String statusResult, String statusMessage,


Why to pass the first three values. They're always the same.

z-sztrom · 2024-07-15T10:47:17Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+
+            JsonObject parsedJson = parsedResponse.getAsJsonObject();
+
+            if (!parsedJson.has(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD)) {


Reverse condition.

z-sztrom · 2024-07-15T10:48:34Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+
+        if (msgService != null) {
+            String response = msgService.generateMsg(msgType, event, isLenientEnabled(okToLeaveOutInvalidOptionalFields));
+            parsedResponse = parser.parse(response);


parse() is marked as deprecated.

Yeah we can use JsonParser() instead of this

z-sztrom · 2024-07-15T10:53:50Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+        } catch (JsonSyntaxException e) {
+            String exceptionMessage = e.getMessage();
+            log.error("Invalid JSON parse data format due to:", e.getMessage());
+            errorResponse.addProperty(RemremGenerateServiceConstants.JSON_STATUS_CODE, HttpStatus.BAD_REQUEST.value());


createResponseEntity() should be used here.

z-sztrom · 2024-07-15T11:00:36Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                    HttpStatus.NOT_ACCEPTABLE, HttpStatus.EXPECTATION_FAILED, HttpStatus.SERVICE_UNAVAILABLE, HttpStatus.UNPROCESSABLE_ENTITY
+            );
+            for (HttpStatus status : statuses) {
+                if (exceptionMessage.contains(Integer.toString(status.value()))) {


I guess there should be more strict test, not just test for occurrence in message. The message is a JSON data.

Yeah but whatever the test is handled by these exception error, it is same like as earlier..

z-sztrom · 2024-07-15T11:03:14Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+
+            JsonObject parsedJson = parsedResponse.getAsJsonObject();
+
+            if (!parsedJson.has(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD)) {


Import constants in order to skip class name when referencing a constant.

z-sztrom · 2024-07-16T14:33:55Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            String exceptionMessage = e.getMessage();
+            log.info("duplicate key detected", exceptionMessage);
+            errorResponse.addProperty(RemremGenerateServiceConstants.JSON_STATUS_RESULT, "fatal");
+            errorResponse.addProperty(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD, "duplicate key detected, please check " + exceptionMessage);


How do you know that this is a duplicate key issue?

z-sztrom · 2024-07-16T14:35:29Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            log.error("Invalid JSON parse data format due to:", e.getMessage());
+            errorResponse.addProperty(RemremGenerateServiceConstants.JSON_STATUS_CODE, HttpStatus.BAD_REQUEST.value());
+            errorResponse.addProperty(RemremGenerateServiceConstants.JSON_STATUS_RESULT, "fatal");
+            errorResponse.addProperty(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD, "Invalid JSON parse data format due to: " + exceptionMessage);


No status code provided?

z-sztrom · 2024-07-17T06:49:51Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            }
+            if (userInputData == null) {
+                log.error("Json event must not be null");
+                errorResponse.addProperty(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD, "userInputData must not be null");


User doesn't know what 'userInputData' is. Use 'Input data' or something like that instead.

z-sztrom · 2024-07-17T06:59:53Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                JsonObject processedJson = processEvent(msgProtocol, msgType, failIfMultipleFound, failIfNoneFound,
+                        lookupInExternalERs, lookupLimit, okToLeaveOutInvalidOptionalFields, inputJsonObject);
+
+                if (processedJson.has(RemremGenerateServiceConstants.META)) {


Simplify result handling as in line 164.

yeah but i checked the flow of code is like that we need the code as it is means need that service_unavailable part here that's why i not do any change regarding this in my latest push

z-sztrom · 2024-07-17T07:30:23Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-            else if (e1.getMessage().contains(Integer.toString(HttpStatus.EXPECTATION_FAILED.value()))) {
-                return new ResponseEntity<>(parser.parse(e1.getMessage()), HttpStatus.EXPECTATION_FAILED);
+                return createResponseEntity(RemremGenerateServiceConstants.JSON_STATUS_CODE, RemremGenerateServiceConstants.JSON_STATUS_RESULT, RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD,
+                        HttpStatus.BAD_REQUEST, "Invalid JSON format,expected either single template or array of templates", "fail", errorResponse, HttpStatus.BAD_REQUEST.value());


A space is missing after comma (,).

z-sztrom · 2024-07-17T07:34:03Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            if (!parsedJson.has(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD)) {
+                return parsedJson;
+            } else {
+                eventResponse.addProperty(RemremGenerateServiceConstants.JSON_STATUS_CODE, HttpStatus.BAD_REQUEST.value());


Use createResponse() method. Please, check if the method can be used at other places.

z-sztrom · 2024-07-18T14:57:37Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            log.error("Invalid JSON parse data format due to:", e.getMessage());
+            return createResponseEntity(HttpStatus.BAD_REQUEST,"Invalid JSON parse data format due to: " + exceptionMessage, "fatal",
+                    errorResponse);
+        } catch (JsonProcessingException e) {


I think JsonSyntaxException and JsonProcessingException can be handled together. No need to distinguish between them.

z-sztrom · 2024-07-18T14:57:50Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+        } catch (JsonProcessingException e) {
+            String exceptionMessage = e.getMessage();
+            log.info("Incorrect Json data", exceptionMessage);
+            return createResponseEntity(HttpStatus.BAD_REQUEST,"Incorrect Json data: " + exceptionMessage, "fatal",


Json -> JSON

z-sztrom · 2024-07-18T14:58:17Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                    errorResponse);
+        } catch (JsonProcessingException e) {
+            String exceptionMessage = e.getMessage();
+            log.info("Incorrect Json data", exceptionMessage);


Should be log.error

z-sztrom · 2024-07-18T14:59:41Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-                if(lookupLimit <= 0) {
-                    return new ResponseEntity<>("LookupLimit must be greater than or equals to 1", HttpStatus.BAD_REQUEST);
+            if (lookupLimit <= 0) {
+                return new ResponseEntity<>("LookupLimit must be greater than or equals to 1", HttpStatus.BAD_REQUEST);


Should createResponseEntity be used?

i don't think here we need, here we just need to return one response not as many like in other scenerio

z-sztrom · 2024-07-18T14:59:54Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            }
+            if (inputData == null) {
+                log.error("Json event must not be null");
+                errorResponse.addProperty(JSON_ERROR_MESSAGE_FIELD, "inputData must not be null");


Should createResponseEntity be used?

same here as well

z-sztrom · 2024-07-18T15:01:52Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                JsonObject processedJson = processEvent(msgProtocol, msgType, failIfMultipleFound, failIfNoneFound,
+                        lookupInExternalERs, lookupLimit, okToLeaveOutInvalidOptionalFields, inputJsonObject);
+
+                if (processedJson.has(META)) {


Handle result as inside loop at line 162.

not getting clearly here,

z-sztrom · 2024-07-18T15:03:02Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-                return new ResponseEntity<>(parser.parse(RemremGenerateServiceConstants.NO_SERVICE_ERROR),
-                        HttpStatus.SERVICE_UNAVAILABLE);
+                return createResponseEntity(HttpStatus.BAD_REQUEST, "Invalid JSON format,expected either single template or array of templates",
+                        "fail", errorResponse);


There should be a constant for "fail".

z-sztrom · 2024-07-18T15:04:30Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            for (HttpStatus status : statuses) {
+                if (exceptionMessage.contains(Integer.toString(status.value()))) {
+                    return createResponseEntity(
+                            status, e.getMessage(), "fail", errorResponse);


Use constant for "fail". The same for lines 216 and 219.

z-sztrom · 2024-07-18T15:10:19Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            JsonObject parsedJson = parsedResponse.getAsJsonObject();
+
+            if (parsedJson.has(JSON_ERROR_MESSAGE_FIELD)) {
+                createResponseEntity(HttpStatus.BAD_REQUEST, "fail", TEMPLATE_ERROR, eventResponse);


Use constants for "fail".

z-sztrom · 2024-07-18T15:12:18Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

        }
+        return eventResponse;


We cannot return an empty JSON object. Situation when msgService == null must be handled.

yeah right we can handle this in same as original one means the how it handle earlier right?

else {
return new ResponseEntity<>(parser.parse(RemremGenerateServiceConstants.NO_SERVICE_ERROR),
HttpStatus.SERVICE_UNAVAILABLE);
}

z-sztrom · 2024-07-24T08:49:06Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                    lookupLimit, okToLeaveOutInvalidOptionalFields, inputJson);
+        } catch (JsonSyntaxException | JsonProcessingException e) {
+            String exceptionMessage = e.getMessage();
+            log.error("Invalid JSON parse data format due to:", e.getMessage());


Remove ':'.

z-sztrom · 2024-07-24T08:49:34Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+     * taking message type and json body as input
+     * Here we basically add this to handle if inputData is of jsonArray type as well
+     *
+     * <p>


Why two '
'?

z-sztrom · 2024-07-24T08:50:17Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            if (inputData == null) {
+                createResponseEntity(HttpStatus.BAD_REQUEST, JSON_ERROR_STATUS,
+                        "Parameter 'inputData' must not be null", errorResponse);
+            }


Why to continue when inputData is null?

z-sztrom · 2024-07-24T08:53:39Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                    return new ResponseEntity<>(processedJson, status);
+                } else if (processedJson.has(JSON_STATUS_CODE)) {
+                    String statusValue = processedJson.get(JSON_STATUS_CODE).toString();
+                    status = HttpStatus.resolve(Integer.parseInt(statusValue));


parseInt can throw NumberFormatException and that should be handled here.

z-sztrom · 2024-07-24T08:57:44Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-            log.error("Unexpected exception caught", e);
-            return new ResponseEntity<>(parser.parse(RemremGenerateServiceConstants.INTERNAL_SERVER_ERROR),
-                    HttpStatus.INTERNAL_SERVER_ERROR);
+            return createResponseEntity(HttpStatus.BAD_REQUEST, e.getMessage(), JSON_ERROR_STATUS, errorResponse);


Why e.getMessage(), it's been stored in exceptionMessage(), the same line 251.

z-sztrom · 2024-07-24T09:02:02Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+     * @param errorResponse   is to collect all the responses here.
+     * @return ResponseEntity
+     */
+    public ResponseEntity<JsonObject> createResponseEntity(HttpStatus status, String responseMessage, String resultMessage,


The method should be also available in form createResponseEntity(HttpStatus, String, String) because errorResponse is usually passed as an empty object. Thus, it can be created internally.

i don't think it is necessary here because we need to store error many responses in one jsonobject , so we need this externally here

Please, dont' forget about this one.

z-sztrom · 2024-07-25T08:57:24Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                    lookupLimit, okToLeaveOutInvalidOptionalFields, inputJson);
+        } catch (JsonSyntaxException | JsonProcessingException e) {
+            String exceptionMessage = e.getMessage();
+            log.error("Invalid JSON parse data format due to", e.getMessage());


Use exceptionMessage instead of e.getMessage().

z-sztrom · 2024-07-25T10:24:24Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

@@ -199,7 +196,7 @@ public ResponseEntity<?> generate(final String msgProtocol, final String msgType
                        "Invalid JSON format,expected either single template or array of templates",
                        JSON_ERROR_STATUS, errorResponse);
            }
-        } catch (REMGenerateException | JsonSyntaxException e) {
+        } catch (REMGenerateException | JsonSyntaxException | NumberFormatException e) {


NumberFormatException cannot be handled that way. It's something completely different then the other ones.

ok we can handle sperately like this right?

try {
status = HttpStatus.resolve(Integer.parseInt(statusValue));
return new ResponseEntity<>(processedJson, status);
} catch (NumberFormatException e){
return new ResponseEntity<>(e.getMessage(),HttpStatus.BAD_REQUEST);
}

z-sztrom · 2024-07-25T10:25:17Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+     * @param errorResponse   is to collect all the responses here.
+     * @return ResponseEntity
+     */
+    public ResponseEntity<JsonObject> createResponseEntity(HttpStatus status, String responseMessage, String resultMessage,


Please, dont' forget about this one.

z-sztrom

Not all newly added methods are documented.

z-sztrom · 2024-07-29T09:50:35Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            String exceptionMessage = e.getMessage();
+            log.error("Invalid JSON parse data format due to", e.getMessage());
+            return createResponseEntity(HttpStatus.BAD_REQUEST, "Invalid JSON parse data format due to: "
+                    + exceptionMessage, "fatal");


Should a constant be used instead of "fatal"?

z-sztrom · 2024-07-29T09:53:45Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-                return new ResponseEntity<>(parser.parse(e1.getMessage()), HttpStatus.UNPROCESSABLE_ENTITY);
+        } catch (NumberFormatException e){
+            String exceptionMessage = e.getMessage();
+            log.error("Invalid number format", exceptionMessage);


This is really confusing for user... What would you do if you were user?

we can logged like this?

log.error("Invalid number, please enter a valid status code value", exceptionMessage);

This is really confusing for user... What would you do if you were user?

we can logged like this?

log.error("Invalid number, please enter a valid status code value", exceptionMessage);

z-sztrom · 2024-07-29T09:55:29Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+        } else if (e instanceof JsonSyntaxException) {
+            log.error("Failed to parse JSON", exceptionMessage);
+            return createResponseEntity(HttpStatus.BAD_REQUEST, exceptionMessage, JSON_ERROR_STATUS);
+        } /*else if (e instanceof NumberFormatException) {


Remove comment.

z-sztrom · 2024-07-29T10:06:45Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

                } else {
-                    return new ResponseEntity<>(parsedResponse, HttpStatus.BAD_REQUEST);
+                    return new ResponseEntity<>(processedJson, HttpStatus.SERVICE_UNAVAILABLE);


This strange state should be logged.

yeah we can logged like this?

log.error("Unexpected error caught, due to service property is missing or wrong");

z-sztrom · 2024-07-29T10:07:41Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+        String exceptionMessage = e.getMessage();
+        if (e instanceof REMGenerateException) {
+            List<HttpStatus> statuses = List.of(
+                    HttpStatus.NOT_ACCEPTABLE, HttpStatus.EXPECTATION_FAILED, HttpStatus.SERVICE_UNAVAILABLE, HttpStatus.UNPROCESSABLE_ENTITY


Split the line.

z-sztrom · 2024-07-30T07:34:04Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                        status = HttpStatus.resolve(Integer.parseInt(statusValue));
+                        return new ResponseEntity<>(processedJson, status);
+                    } catch (NumberFormatException e) {
+                        log.error("Invalid status value: '" + statusValue + "' of response " + processedJson);


Why to compose the same string twice? Store it in a variable and reuse it. The same 195 and 196.

Can do like this?

else {
String errorMessage = "There is no status value in the response " + processedJson;
log.error(errorMessage);
return createResponseEntity(HttpStatus.INTERNAL_SERVER_ERROR, errorMessage, JSON_ERROR_STATUS);
}

Yes, that's perfect solution.

z-sztrom · 2024-07-30T07:34:25Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

                } else {
-                    return new ResponseEntity<>(parsedResponse, HttpStatus.BAD_REQUEST);
+                    log.error("There is no status value: '" + statusValue + "' in the response" + processedJson);


Missing a space after 'response'.

Missing a space after 'response'.

OK

z-sztrom · 2024-07-30T07:38:29Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-                    return new ResponseEntity<>(parsedResponse, HttpStatus.BAD_REQUEST);
+                    log.error("There is no status value: '" + statusValue + "' in the response" + processedJson);
+                    return createResponseEntity(HttpStatus.INTERNAL_SERVER_ERROR, "There is no status value: '"
+                            + statusValue + "' in the response " + processedJson, JSON_ERROR_STATUS);


statusValue is always null here. You should move its declaration from 180 to 185.

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

shudhansu-shekhar

For your two specific comment, we can do like this:

catch (NumberFormatException e) {
String errorMessage = "Invalid status value: '" + statusValue + "' of response " + processedJson;
log.error(errorMessage);
return createResponseEntity(HttpStatus.BAD_REQUEST, err, JSON_ERROR_STATUS);
}
} else {
String errorMessage = "There is no status value in the response " + processedJson;
log.error(errorMessage);
return createResponseEntity(HttpStatus.INTERNAL_SERVER_ERROR, errorMessage, JSON_ERROR_STATUS);
}

shudhansu-shekhar · 2024-07-02T10:23:06Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-            else {
-                return new ResponseEntity<>(parser.parse(e1.getMessage()), HttpStatus.UNPROCESSABLE_ENTITY);
+        } catch (REMGenerateException e) {
+            if (e.getMessage().contains(Integer.toString(HttpStatus.NOT_ACCEPTABLE.value()))) {


sorry but inside every if condition is doing different job and it seems there is some confusion here, it seems you provide comment on older one?

shudhansu-shekhar · 2024-07-02T10:27:03Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+
+            if (msgService != null) {
+                String response = msgService.generateMsg(msgType, event, isLenientEnabled(okToLeaveOutInvalidOptionalFields));
+                parsedResponse = parser.parse(response);


but how if there is response then how parsedResponse will be null?

shudhansu-shekhar · 2024-07-02T10:29:00Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+                String response = msgService.generateMsg(msgType, event, isLenientEnabled(okToLeaveOutInvalidOptionalFields));
+                parsedResponse = parser.parse(response);
+
+                if (!parsedResponse.getAsJsonObject().has(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD)) {


yeah but where i can't find like this where you suggest?

shudhansu-shekhar · 2024-07-02T10:29:53Z

.../test/java/com/ericsson/eiffel/remrem/generate/integrationtest/EiffelRemremControllerIT.java

@@ -173,7 +173,7 @@ public void testDuplicateKeyInBody() throws IOException {
                .contentType("application/json")
                .body(activityFinishedDuplicateKeysBody)
                .when()
-                    .post("/eiffelsemantics?msgType=EiffelActivityFinishedEvent")
+                    .post("/eiffelsemantics?msgType=EiffelActivityFinishedEven")


yeah sorry i just testing with this....i will do this

shudhansu-shekhar · 2024-07-02T10:32:25Z

...rc/test/java/com/ericsson/eiffel/remrem/generate/service/EiffelRemremControllerUnitTest.java

        assertEquals(elem.getStatusCode(), HttpStatus.BAD_REQUEST);
    }

+    @Test
+    public void testEiffel3FailureEventArray() throws Exception {


yes....but we can use this real event?

shudhansu-shekhar · 2024-07-29T11:14:33Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+        String exceptionMessage = e.getMessage();
+        if (e instanceof REMGenerateException) {
+            List<HttpStatus> statuses = List.of(
+                    HttpStatus.NOT_ACCEPTABLE, HttpStatus.EXPECTATION_FAILED, HttpStatus.SERVICE_UNAVAILABLE, HttpStatus.UNPROCESSABLE_ENTITY


shudhansu-shekhar · 2024-07-29T11:17:47Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+            String exceptionMessage = e.getMessage();
+            log.error("Invalid JSON parse data format due to", e.getMessage());
+            return createResponseEntity(HttpStatus.BAD_REQUEST, "Invalid JSON parse data format due to: "
+                    + exceptionMessage, "fatal");


shudhansu-shekhar · 2024-07-29T11:22:49Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

                } else {
-                    return new ResponseEntity<>(parsedResponse, HttpStatus.BAD_REQUEST);
+                    return new ResponseEntity<>(processedJson, HttpStatus.SERVICE_UNAVAILABLE);


yeah we can logged like this?

log.error("Unexpected error caught, due to service property is missing or wrong");

shudhansu-shekhar · 2024-07-29T11:28:33Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-                return new ResponseEntity<>(parser.parse(e1.getMessage()), HttpStatus.UNPROCESSABLE_ENTITY);
+        } catch (NumberFormatException e){
+            String exceptionMessage = e.getMessage();
+            log.error("Invalid number format", exceptionMessage);


we can logged like this?

log.error("Invalid number, please enter a valid status code value", exceptionMessage);

shudhansu-shekhar · 2024-07-29T11:54:34Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

-                return new ResponseEntity<>(parser.parse(e1.getMessage()), HttpStatus.UNPROCESSABLE_ENTITY);
+        } catch (NumberFormatException e){
+            String exceptionMessage = e.getMessage();
+            log.error("Invalid number format", exceptionMessage);


This is really confusing for user... What would you do if you were user?

we can logged like this?

log.error("Invalid number, please enter a valid status code value", exceptionMessage);

shudhansu-shekhar

Please check my comment

shudhansu-shekhar · 2024-08-02T04:37:50Z

...e/src/main/java/com/ericsson/eiffel/remrem/generate/controller/RemremGenerateController.java

+     * @param errorResponse   is to collect all the responses here.
+     * @return ResponseEntity
+     */
+    public ResponseEntity<JsonObject> createResponseEntity(HttpStatus status, String responseMessage, String resultMessage,


z-sztrom

Please, add some examples of multiple responses, so user can see how to submit request and what kind of response can be expected.

z-sztrom · 2024-08-08T13:07:51Z

wiki/markdown/usage/service.md

+| 422         | Unprocessable Entity  | Link specific lookup options could not be fulfilled                                                                           | Is returned if Link specific lookup options could not be matched with failIfMultipleFound and failIfNoneFound.                                                                  |
+| 500         | Internal Server Error | Internal server error                                                                                                         | When REMReM Generate is down, possible to try again later when server is up                                                                                                     |
+| 503         | Service Unavailable   | "No protocol service has been found registered"                                                                               | When specified message protocol is not loaded                                                                                                                                   |
+| 207         | Multi Status          | Multi-Status                                                                                                                  | Is returned when there's a partial success, i.e. some events failed, some successfully generated/published, return status should be 207.                                        |


Please, move after 200 status.

z-sztrom · 2024-08-08T13:08:22Z

wiki/markdown/usage/service.md

+| 422         | Unprocessable Entity  | Link specific lookup options could not be fulfilled                                                                           | Is returned if Link specific lookup options could not be matched with failIfMultipleFound and failIfNoneFound.                                                                  |
+| 500         | Internal Server Error | Internal server error                                                                                                         | When REMReM Generate is down, possible to try again later when server is up                                                                                                     |
+| 503         | Service Unavailable   | "No protocol service has been found registered"                                                                               | When specified message protocol is not loaded                                                                                                                                   |
+| 207         | Multi Status          | Multi-Status                                                                                                                  | Is returned when there's a partial success, i.e. some events failed, some successfully generated/published, return status should be 207.                                        |


This is generate service, don't use generate/publish here.

z-sztrom · 2024-08-09T12:53:04Z

wiki/markdown/usage/service.md

@@ -139,7 +139,133 @@ Status codes are generated according to the below table.
 | 503         | Service Unavailable   | "No protocol service has been found registered"                                                                               | When specified message protocol is not loaded                                                                                                                                   |
 | 207         | Multi Status          | Multi-Status                                                                                                                  | Is returned when there's a partial success, i.e. some events failed, some successfully generated/published, return status should be 207.                                        |

+### Examples:


Please, provide responses, so user can see how the outputs are structured.

z-sztrom · 2024-08-09T12:53:43Z

wiki/markdown/usage/service.md

@@ -139,7 +139,133 @@ Status codes are generated according to the below table.
 | 503         | Service Unavailable   | "No protocol service has been found registered"                                                                               | When specified message protocol is not loaded                                                                                                                                   |
 | 207         | Multi Status          | Multi-Status                                                                                                                  | Is returned when there's a partial success, i.e. some events failed, some successfully generated/published, return status should be 207.                                        |


Please, move 207 after 200 status.

z-sztrom · 2024-08-12T11:03:13Z

wiki/markdown/usage/service.md

@@ -139,6 +139,10 @@ Status codes are generated according to the below table.
 | 503         | Service Unavailable   | "No protocol service has been found registered"                                                                               | When specified message protocol is not loaded                                                                                                                                   |
 | 207         | Multi Status          | Multi-Status                                                                                                                  | Is returned when there's a partial success, i.e. some events failed, some successfully generated/published, return status should be 207.                                        |

+
+NOTE: In the array of event if any of the event have not correct or valid JSON format,


Remove 'correct or'.

z-sztrom · 2024-08-12T11:04:50Z

wiki/markdown/usage/service.md

+      }
+    ]
+  },
+{


Please, fix the indentation.

shudhansu-shekhar

Fix all the suggestion or comment as well and push the changes. Please check the comment

shudhansu-shekhar · 2024-08-12T13:35:43Z

wiki/markdown/usage/service.md

@@ -139,6 +139,10 @@ Status codes are generated according to the below table.
 | 503         | Service Unavailable   | "No protocol service has been found registered"                                                                               | When specified message protocol is not loaded                                                                                                                                   |
 | 207         | Multi Status          | Multi-Status                                                                                                                  | Is returned when there's a partial success, i.e. some events failed, some successfully generated/published, return status should be 207.                                        |

+
+NOTE: In the array of event if any of the event have not correct or valid JSON format,


shudhansu-shekhar · 2024-08-12T13:36:00Z

wiki/markdown/usage/service.md

@@ -139,6 +139,10 @@ Status codes are generated according to the below table.
 | 503         | Service Unavailable   | "No protocol service has been found registered"                                                                               | When specified message protocol is not loaded                                                                                                                                   |
 | 207         | Multi Status          | Multi-Status                                                                                                                  | Is returned when there's a partial success, i.e. some events failed, some successfully generated/published, return status should be 207.                                        |

+
+NOTE: In the array of event if any of the event have not correct or valid JSON format,


shudhansu-shekhar

done, please check comment

z-sztrom

Looks good, tests passed.

Modified generate's endpoint generate to accept array of events

6eb83b3

z-sztrom requested changes Jun 19, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

d33c54c

z-sztrom requested changes Jul 2, 2024

View reviewed changes

shudhansu-shekhar added 2 commits July 9, 2024 15:18

Modified generate's endpoint generate to accept array of events

1476f2d

Modified generate's endpoint generate to accept array of events

e58d75b

z-sztrom requested changes Jul 15, 2024

View reviewed changes

z-sztrom requested changes Jul 16, 2024

View reviewed changes

z-sztrom requested changes Jul 17, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

55d720d

z-sztrom requested changes Jul 18, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

037c273

z-sztrom requested changes Jul 24, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

89b52ef

z-sztrom requested changes Jul 25, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

145f2b8

z-sztrom requested changes Jul 29, 2024

View reviewed changes

z-sztrom reviewed Jul 29, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

dfa1799

z-sztrom requested changes Jul 30, 2024

View reviewed changes

shudhansu-shekhar commented Jul 30, 2024

View reviewed changes

shudhansu-shekhar added 2 commits July 30, 2024 15:02

Modified generate's endpoint generate to accept array of events

7ea6195

Modified generate's endpoint generate to accept array of events

3a3fd1c

shudhansu-shekhar commented Aug 2, 2024

View reviewed changes

shudhansu-shekhar added 3 commits August 2, 2024 14:24

Modified generate's endpoint generate to accept array of events

962cabb

Modified generate's endpoint generate to accept array of events

dbb2ed3

Modified generate's endpoint generate to accept array of events

ddca4e6

z-sztrom reviewed Aug 8, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

b5ed6b4

z-sztrom requested changes Aug 9, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

cd2a736

z-sztrom requested changes Aug 12, 2024

View reviewed changes

shudhansu-shekhar commented Aug 12, 2024

View reviewed changes

Modified generate's endpoint generate to accept array of events

6456f81

z-sztrom approved these changes Aug 14, 2024

View reviewed changes

shudhansu-shekhar merged commit a870ce4 into eiffel-community:master Aug 14, 2024
1 check passed


		JsonObject parsedJson = parsedResponse.getAsJsonObject();

		if (!parsedJson.has(RemremGenerateServiceConstants.JSON_ERROR_MESSAGE_FIELD)) {

		@@ -139,7 +139,133 @@ Status codes are generated according to the below table.
		\| 503 \| Service Unavailable \| "No protocol service has been found registered" \| When specified message protocol is not loaded \|
		\| 207 \| Multi Status \| Multi-Status \| Is returned when there's a partial success, i.e. some events failed, some successfully generated/published, return status should be 207. \|

Modified generate's endpoint generate to accept array of events #226

Modified generate's endpoint generate to accept array of events #226

Conversation

shudhansu-shekhar commented Jun 19, 2024 • edited Loading

Applicable Issues

Description of the Change

Alternate Designs

Possible Drawbacks

Sign-off

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

Choose a reason for hiding this comment

shudhansu-shekhar Jun 20, 2024 • edited Loading

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

z-sztrom left a 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

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

z-sztrom left a 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

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

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

shudhansu-shekhar commented Jun 19, 2024 •

edited

Loading

shudhansu-shekhar Jun 20, 2024 •

edited

Loading