Validate and raise error when include directives are invalid?

[kinopyo]

I found the change was made in https://github.com/beauby/jsonapi/pull/23 initially:

By passing them through the parser an error can be raised when the includedirectives are validated. If the parser strips them out we lose the ability to raise an error.

My question is, should we provide a validation method to check all keys and raise an error for invalid ones? (or is there already one but I missed..? It seems like the #key? method can check single one)

For example:

# intention is to include `friends`, `comments`, `posts` 
JSONAPI::IncludeDirective.new("friends, comments, posts").validate
# => InvalidKeyError: ' comments' and ' posts' are invalid.

[beauby]

Hi @kinopyo – thanks for raising this concern. I believe the right behavior would be to raise an exception during the parsing of the value of an ill-formed include query parameter. What do you think?

[kinopyo]

@beauby Hi thanks for the reply! That sounds even better - raising an exception during the parsing(initialization) of the values! I can come up with a PR later 🙂

[beauby]

@kinopyo That would be awesome 👍

[zhouqing86]

@kinopyo @beauby Rather than throw Invalid exception, why we don't just handle(for example: eliminate spaces) to make it robust, is there any concern to handle string with spaces rather than validate string with spaces? I just submit an issue here: https://github.com/jsonapi-rb/jsonapi-renderer/issues/18

[beauby]

@zhouqing86 The JSON API spec states

The value of the include parameter MUST be a comma-separated (U+002C COMMA, “,”) list of relationship paths.

So I think it is better to throw an exception since a space after a comma in that context is either a misinterpretation of the spec or a possible bug, which is better caught sooner rather than later.

[zhouqing86]

Thanks @beauby for your quick response!

I originally submit an issue on active_model_serializers which are using jsonapi-renderer. https://github.com/rails-api/active_model_serializers/issues/2139

I am agree with your concern that we need to follow JSON API spec. Thanks again!