mattpolzin / JSONAPI

Swift Codable JSON:API framework
MIT License
75 stars 19 forks source link

Clearer include failure errors #86

Closed mattpolzin closed 4 years ago

mattpolzin commented 4 years ago

Closes https://github.com/mattpolzin/JSONAPI/issues/84.

Old messages mentioned the index of the failed include (i.e. of all the includes in the document, which one failed) and also a message for why the given include was not of each possible type of include.

Include 3 failed to parse: Could not have been Include Type 1 because: found JSON:API type "not_an_author" but expected "articles"

Could not have been Include Type 2 because: found JSON:API type "not_an_author" but expected "authors"

The new message after this PR will display a verbose message like above (with a bit more clarity) if needed but in the common cases of (a) all failures have the wrong JSON:API type or (b) exactly one failure has the correct JSON:API type a much more concise and targeted error message will be generated.

mlomeli commented 4 years ago

👍 Thanks :). This is clearer. I see now the logic behind how it was named before.

the problem is something else like a missing attribute the error message still reads as crystal clear (I think).

I think that if the problem is a missing attribute, shouldn't the error message only contain that information? Since it will create an Exception and make all the other Includes fail? Or is it possible to have an error with Two Include errors due to missing attributes?

mattpolzin commented 4 years ago

@mlomeli thanks for taking a look here and confirming that these changes make the error message clearer.

I think that if the problem is a missing attribute, shouldn't the error message only contain that information?

Perhaps you are right. Here's a bit of thinking on where we could go from here to further improve the conciseness of the errors for include parsing:

An example of an error with a missing attribute after this PR is merged would be:

Out of 3 includes, the 3rd one failed to parse: Could not have been Include Type entity1 because: found JSON:API type "entity2" but expected "entity1"

Could not have been Include Type entity2 because: 'foo' attribute is required and missing.

Could not have been Include Type entity3 because: found JSON:API type "entity2" but expected "entity3"

Which, if I were to rewrite as a single sentence, would say

The third include could not be decoded as a entity1 because the JSON:API type didn't match; it couldn't be decoded as a entity2 because there is no 'foo' attribute; and it could not be decoded as a entity3 because the JSON:API type didn't match.

This error would occur in a situation where the Include3<Entity1,Entity2,Entity3> type was used which is why the error indicates the reason why the particular include could not be decoded as any of those 3 types.

I do see room for improvement here. There is enough information to boil it down further.

If all of the errors are type-related, the error could be much more concise. For example:

Out of 10 includes, the 2nd one failed to parse: The JSON:API type found_this does not match any of the include types (entity1, entity2).

If all of the errors except for one are type-related, then it is safe to assume the only relevant error is the one that had the right type but some other problem. For example:

Out of 5 includes, the 4th one failed to parse: 'foo' attribute is required and missing.

mattpolzin commented 4 years ago

@mlomeli I think I got this to a good place with the last push there. Now the common cases of (a) the JSON:API type does not match any of the allowed values and (b) there is exactly one failure with the correct JSON:API type are both represented much more concisely but it still falls back to a more verbose message enumerating all the failures otherwise.