Add FormFieldValidationException Catching in the API

[StephenOTT]

Currently has no tests Looking for starter feedback before adding the tests.

Throws a 400 because it is a form validation and not a actual server error (500). Aka: the server processed the input correctly, but the input did not pass the "form validations"

Work was based on https://app.camunda.com/jira/browse/CAM-8276

[koevskinikola]

Hey @StephenOTT ,

I just started reviewing your PR so I'll have some feedback for you by the end of the day.

Cheers,
Nikola

[koevskinikola]

Hey @StephenOTT,

Sorry for not responding earlier, we're having the 7.9 minor release this month and I got sidetracked with that.

Everything looks good in the code you contributed, only the tests are missing.

For me, it makes most sense to add the tests to TaskRestServiceInteractionTest. There are already several ones that test the complete endpoint, so you would just need to add a single test case in my opinion.

Best,
Nikola

[StephenOTT]

@koevskinikola tests have been added.

Original code sample was also modified as it was in the wrong section. It was originally in the Complete Task, rather than in the Submit Task methods.

Code has been updated for Process Definition submit-form and User Task submit-form endpoints.

Tests have been added that match the submit form tests.

within the engine-rest folder you can run

mvn clean test -Pjersey -Dtest=org.camunda.bpm.engine.rest.ProcessDefinitionRestServiceInteractionTest

and

mvn clean test -Pjersey -Dtest=org.camunda.bpm.engine.rest.TaskRestServiceInteractionTest

[StephenOTT]

@koevskinikola I had another thought for a improvement: for the error messages as for example: https://github.com/camunda/camunda-bpm-platform/pull/296/files#diff-508062d5c5e734d5b4ba478a79f0beacR191

With the FormFieldValidationException object that is caught, we have access to the "detail" Object and the "message String.

We could use the message as it is currently being used, But if the proper Detail object is provided, such as a String with a specific value, the final message would be replaced by the entirety of the message that was passed into exception.

Why do this? For Client side error handling.

the caught exception is for throwing form validation errors. Therefore there are validations we want to pass back to the client (such as which fields were incorrectly submitted).

Basically we would go from something like:

Cannot instantiate process definition someProcessId: ${the thrown exception message}

and instead just return:

${the thrown exception message}

which would just end up being a JSON object or array. It would be up to the code that chose to throw the formfieldvalidationexception on whether to issue the exception with the "keyword" or without. If they did not use the proper keyword, the normal value would be used.

In practice the message node in the RestException response would just be Stringified JSON that the client side would be responsible for parsing before interpreting.

This is obviously a bit of a injection of functionality into the message node of the RestException response. But IMO it's something that is in the control of the developer who choses to throw the formFieldValidationException, and thus the string result vs the json message result would be known expectations.

Thoughts?

[StephenOTT]

@koevskinikola see: dbdfdc1

I have added the additional logic that looks for the keyword.

For review and consideration.

If someone creates the "details" object with the value "json-validation" then it will return just the raw message rather than the normal appending of "Cannot submit task + ${message}" and "Cannot start process instance + ${message}"

"json-validation" was chosen because the general usecase is that the user wants to return json as part of the response due to Form Validation values typically being json based.

@felix-mueller might have some insight as it was his original issue in JIRA from: CAM-8276

[StephenOTT]

@koevskinikola any feedback on this? With the json-validation enhancement acceptable?

[koevskinikola]

Hey @StephenOTT ,

I have been out of office for the past week and haven't had a chance to look at the changes/additions. I'll have a look at them today or tomorrow and give you feedback.

Best,
Nikola

[koevskinikola]

Hey @StephenOTT,

I don't think the json-validation enhancement would be a good idea.

1. The detail is hard-coded, so users would have to be made aware that the Exception Detail needs to be json-validation in order for this to work. It would also limit them to use only that Detail label if they want to gain that feature (what if they want to set the Exception Detail to something else and still have the feature?).
2. Currently, the whole exception message is already contained in the final message, the users just need to parse it for client side error handling. If there are two possible formats of the final message, this would also have to be handled on the client side, adding to the complexity.
3. This is also related to [2]. The final message of the enhancement differs from the pattern that we have for the rest of the handled exceptions (ProcessEngineException, RestException), so extra logic on the client side would have to be introduced in order to handle the additional format of the FormFieldValidationException.

Would this enhancement make a big (positive) difference when developing on the client side?

Best,
Nikola

[StephenOTT]

The detail is hard-coded, so users would have to be made aware that the Exception Detail needs to be json-validation in order for this to work. It would also limit them to use only that Detail label if they want to gain that feature (what if they want to set the Exception Detail to something else and still have the feature?).

In this specific example, it would create a single narrow restriction: If someone wanted to provide custom message they would have to give up the detail field. This is just one way to implement this in the form of a "switch", but it would be other things like detecting that the message is valid JSON. Lets not get bogged down on the specific implementation just yet.

---

Currently, the whole exception message is already contained in the final message, the users just need to parse it for client side error handling. If there are two possible formats of the final message, this would also have to be handled on the client side, adding to the complexity.

Yes they would. The client side can detect if the message node is a Json content or string. BUT without this you are assuming that the client side always wants to receive human readable sentence, which is not a valid assumption for client side form validations. Form validations are returned with a array of validation errors, and each error can be placed on the specific field that field.

---

This is also related to [2]. The final message of the enhancement differs from the pattern that we have for the rest of the handled exceptions (ProcessEngineException, RestException), so extra logic on the client side would have to be introduced in order to handle the additional format of the FormFieldValidationException.

Yes agreed. Somewhere someone is adding new client side logic: but that is the purpose of enhancement: to enable the client side to do things with the actual message being returned. The message that is returned is currently only useful for a human to read. What we want to be able to do with client side validations is return a object that the client side app can read, parse, make sense of, and present the errors in the proper format for the end user.
We are talking about multiple form validations being returned in a single response. This is different than the current practice of the other form validators being used, as they all involve single validations on a per field basis.

---

Would this enhancement make a big (positive) difference when developing on the client side?

IMO, yes, because it allows the client side to actually use the form validation failures and present the errors in the proper location.
It is the equivalent of having a form with 3 fields where you type in some numbers. The Server validates each of the three fields and returns errors for the fields that were incorrect. In the correct scenario that is implemented in camunda it would return a message such as:

Cannot submit task 312-4432-432-3432-324-3: the field FieldA has a incorrect value, and the field Field C has a incorrect value

You would have to write up a function to build the human readable error sentence to make sense of the error and it is only valuable for displaying the error for a human to read; no contextual error is possible to provide accessible (WCAG) error messages around the specific field that is throwing the error.

The client side is expecting a object or a array of validation errors, and those errors are parsed and processed by the client side.

---

If this is a sticking point, how about we roll back the json-validation change, commit the previous changes for catching the form validation and then I will open a new PR for how to handle returning machine readable form validation errors?

[koevskinikola]

Hi @StephenOTT,

You make a good point. I had a talk with my colleagues regarding the json-validation change, and while we think that providing a json response would be good, it would need to be implemented more globally and not just for this specific case.

So, as you suggested, we can roll back the json-validation change and merge the contribution. You can then open an issue, or a topic on the forum where we can have a larger discussion on the types of response formats that should be provided by the Rest API.

Best,
Nikola

[StephenOTT]

@koevskinikola can you provide some detail into what "global" means in this context? The specific use-case is fairly specific to Form Validations and not global to the engine.

There are many ways to accomplish this goal; thus why i did not want to get bogged down in the specific "json-validation" text approach, and agree that its a needed feature for form validation errors, and then cycle around what implementation works best without having to change a 100 things.

Different thoughts I had were:

1. Enable the detail object to be returned in the rest api as a additional json key using a flag in the Form Validation exception method.

2. Create a additional object in the Form Validation Exception that, when not null, is returned as a additional json key in the Rest Exception (a variation of the above).

In both examples, the restException would need to be modified to support adding additional json nodes to the response.

[StephenOTT]

@koevskinikola The "json-validation" commits have been reverted.

[StephenOTT]

Forum conversation has been opened as well:

https://forum.camunda.org/t/enhancing-form-field-validation-with-machine-readable-responses/7462?u=stephenott

[koevskinikola]

Hey @StephenOTT,

That's great, I just merged your contribution with the master. I squashed your commits and formatted the commit message to fit the project guidelines.

Regarding your question about what "global" means in this context, that's also something that needs to be discussed, whether only some specific endpoint need to be changed, or if the whole Rest API should support machine readable responses. Additionally, Exception responses can be handled by extending the JsonMappingExceptionHandler, so users can implement your enhancement through there. That's another thing that we have to consider.

Furthermore, if we're making some changes to how Rest Exceptions are handled by default, we have to be really carefull that the changes don't break some existing client implementations and remain backwards compatible.

I'll leave further discussions for the forum. :)

Either way, thank you for the contribution!

Best,
Nikola

[StephenOTT]

👍 thanks