search

witness-org / witness

Your fitness witness!
1 stars 0 forks source link

60: Include Error-Key in All JSON Error Responses - [merged] #89

Closed raffaelfoidl closed 2 years ago

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 18, 2022, 24:44

Merges 60-include-error-key-in-all-json-error-responses -> develop

Closes #60.
Should be merged after !21

New for validation errors: To be consistent with other Spring errors, BAD REQUEST is now lowercased and we include the request path:
image

The main contribution of this ticket: For all other errors wrapped by spring (with ServerException or ServerRuntimeException as cause) include those fields too:
image

There is (to my knowledge) only one exception to this statement: the regular "forbidden" error - because it's not caused by an exception in the first place, but by a Spring filter. One would have to create a @ControllerAdvice and hijack the correct base implementation. And I don't care about this use case enough to do it with this ticket.
image

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 18, 2022, 02:02

added 1 commit

Compare with previous version

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 19, 2022, 13:14

requested review from @11712616

raffaelfoidl commented 2 years ago

In GitLab by @11712616 on Jan 24, 2022, 13:41

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 193

"applicable to given exercise"

raffaelfoidl commented 2 years ago

In GitLab by @11712616 on Jan 24, 2022, 13:41

Commented on server/src/main/java/com/witness/server/service/impl/WorkoutLogServiceImpl.java line 270

Should probably be ServerError.EXERCISE_LOG_NOT_FOUND.

raffaelfoidl commented 2 years ago

In GitLab by @11712616 on Jan 24, 2022, 13:41

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 148

Is this error really dependent on the name specifically? The documentation of all other related errors is about the identifier.

raffaelfoidl commented 2 years ago

In GitLab by @11712616 on Jan 24, 2022, 13:41

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 158

"the current request method"
Also, with "request method" I associate GET, POST etc. Maybe omit the "method"? Or did you include it for a specific reason? If so, what was the reason?

raffaelfoidl commented 2 years ago

In GitLab by @11712616 on Jan 24, 2022, 13:41

Commented on server/src/main/java/com/witness/server/web/infrastructure/ValidationFailureControllerAdvice.java line 104

"advice" instead of "device"?

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 193

changed this line in version 2 of the diff

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/service/impl/WorkoutLogServiceImpl.java line 270

changed this line in version 2 of the diff

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 158

changed this line in version 2 of the diff

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/web/infrastructure/ValidationFailureControllerAdvice.java line 104

changed this line in version 2 of the diff

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

added 4 commits

Compare with previous version

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 193

-> c0e1ac5d7b6f4b356eba76473996b33d6b875210

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/service/impl/WorkoutLogServiceImpl.java line 270

-> 4e41b37891430c57bbc57b9896dc51087573a94e

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 148

Yes, we don't allow users to create exercises with duplicate names (ExerciseServiceImpl):

image

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 158

You are right regarding "request method".
-> 7d82e05c89426ba4da0ee246c01096ad8d633a9d

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:14

Commented on server/src/main/java/com/witness/server/web/infrastructure/ValidationFailureControllerAdvice.java line 104

:face_palm:
-> 638098e1ac949bf0dd884e4cae6636192a1d55c3

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:19

marked this merge request as ready

raffaelfoidl commented 2 years ago

In GitLab by @11712616 on Jan 24, 2022, 14:28

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 148

But what's the difference to the respective error for initial exercises, for example? That one is thrown if an admin tries to create an initial exercise with a name that is already "assigned" to another initial exercise, basically the same behavior as for the user exercises. Why reference the identifier in the documentation of one error and the name in the documentation for the other? I'm sorry, I don't get it. :(

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:37

added 1 commit

Compare with previous version

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 14:37

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 148

Hm, that's because the documentation of the other one is wrong.
-> cc329990843f6c515493cc88bc3d6bdea760c8a6

But what else do you mean? You said "all other related errors" - I checked and other related errors are directly referring to "UID", "email" or the like - so what other instances of name vs identifier are you not content with?

raffaelfoidl commented 2 years ago

In GitLab by @11712616 on Jan 24, 2022, 14:51

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 148

Honestly, I think I simply misunderstood the meaning of "identifier". You mean the concrete ID, right? I somehow thought that it has a more "general" meaning, i.e. it is a way of saying that "the set value already exists for the used means of identification". For example, if I try to create a new exercise with an already taken name, the "identifier" refers to the name. If there are other fields that have to be unique, then the error can be reused without having to adjust the documentation (if we were to use "identifier" this way - which I'm not suggesting, just for clarification). But forget it, it was just a misunderstanding on my side.

Thanks for correcting the documentation!

raffaelfoidl commented 2 years ago

In GitLab by @11712616 on Jan 24, 2022, 14:51

approved this merge request

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 17:18

resolved all threads

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 17:22

added 3 commits

Compare with previous version

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 17:23

added 1 commit

Compare with previous version

raffaelfoidl commented 2 years ago

In GitLab by @11775820 on Jan 24, 2022, 17:47

mentioned in commit 14eecfcd63680c455dd094aad37a619035926dba