find with nil attribute

[danielwheeler1987]

Hello, so I have a concern related to the find method. I noticed that if for some reason the specified id for model record is nil and passed into the api find method it will interpolate the nil on the end of the query string when performing the request. This means the index action instead of the show action is called and returns the entire collection of specified model records. Wouldn't it be ideal for the api client lib to handle this nil behavior and throw a client error (argument error)? I would really like some feedback on this. I would prefer not to have to...

1. Fork the lib.
2. Handle nil checks via the consuming application.
3. Write some custom hacks to handle checks in client wrappers.

Thanks, and TGIF :)

[gaorlov]

Can you tell me a little bit more about what you're seeing? Does it hit http://api.example.com/models/nil, http://api.example.com/models?id=nil, or http://api.example.com/models? Can you also tell me a bit more about the use case when you're looking for a record with a nil primary key and what the expected behavior is?

[stokarenko]

@gaorlov
In short, it hits http://api.example.com/models.

But the problem can be a critical for specific scenarios:

1
That is a bit curios, but json_api_client returns the array from find method, always.
The history of this fact was discussed here - #75

So, when we searching for a single resource by primary key, we typically write the things like

product = Product.find(id).first

2
The next thing which we need to notice - json_api_client will just interpolate the incoming find param to the end of API URL, just like that

http://somehost/api/v1/products/{id}

3
What will happen if we pass the blank id (nil or empty string) to the find method then?.. Ya, json_api_client will try to call the INDEX API endpoint instead of SHOW

http://somehost/api/v1/products/

Lets sum 1 + 2 + 3 - in case if id is blank, we will silently receive the product variable equal to some existing resource, with all the consequences.

Even worse, product variable will equal to random resource, it will be not Idempotent.

Once again - at the moment we receive the random resource when searching by blank primary key O_o

That is absolutely not what we expect to reach. That can give us a tons of unexpected and unexplained effects all around the system.

To fix that we patched the builder this way:

class MyBuilder < JsonApiClient::Query::Builder
      def find(args={}, collection: false)
        raise ::JsonApiClient::Errors::NotFound if !collection && args.blank?

        super(args)
      end

      def to_a
        @to_a ||= find(collection: true)
      end
      alias all to_a
end

Please let me know is that looks good enough for PR :)

[senid231]

In case of singleton api endpoint current behaviour is correct.
I think we can add optional configuration on resource class level for that

[stokarenko]

@senid231 Roger, just thought the same
Going to prepare PR draft soon )

[stokarenko]

@senid231 @gaorlov here we go - #344

[gaorlov]

@stokarenko marvelous. I really appreciate you doing this work! I left one small comment, but it looks solid otherwise. Thanks!

[stokarenko]

@gaorlov force pushed with additional test )

[gaorlov]

1.11.0 is live. Thanks for the contribution, @stokarenko