-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Where with the primary key results in a find #93
Comments
@johan-smits Ah, I think in order for that to happen, you will need to drop down to a custom request (some examples found here). The intended behavior of Spyke is to “fill out” placeholders defined in the URI (ie Recipe.with(“users/:user_id/recipes”).where(user_id: 3) # => /users/3/recipes For your situation I guess you could do something like: Recipe.with(“recipes”).where(id: [1,2]) Hope that clarifies things? |
IMO, while his makes sense if you look at the implementation (also because |
@balvig I understand the examples but it is counter intuitive. When for example the recipes endpoint changes from its location I have to modify it from the model and all locations that I use this in the code. |
@paulvt and @johan-smits I probably don't fully understand the problems you're running into yet 🤔
Ah really sorry, but am having a hard time picturing this actual problem! 🙇 Gut feeling is that if we for example make class Recipe
uri "users/:user_id/recipes/:id"
end
Recipe.where(user_id: 4).find(6) #=> /users/:user_id/6?user_id=4 |
Yes, for chained associations this becomes more of a problem. I think that Her has no issue with this because it has a separate notion of collection and resource URI. Obviously, then, class User
has_many :recipes, uri: "users/:user_id/recipes"
end
class Recipie
uri "users/:user_id/recipes/(:id)"
end
user = User.find(4)
user.recipes.find(6) # Use resource URI users/:user_id/recipes/:id
user.recipes.where(id: 6) # Use collection URI users/:user_id/recipes, still fill in :user_id However, for Spyke, where uses |
The problem is mainly for the users of our library, who would expect to be able to do:
not known anything about the underlying URIs of the Spyke classes. |
Ah, maybe I made things more confusing by using something resembling an association as an example 🙇 In your example above, using I guess as I developer I would also expect I suppose this would force the developer to always have to go through a Maybe it helps to think of something that’s not an association, I would be surprised if this didn’t work for example: class Recipe
uri “:cuisine/recipes/(:id)”
end
Recipe.where(cuisine: “french”).find(5) #=> /french/recipes/5 I’m not completely understanding how using Do you have a proposal on how you imagine this should work? I think there’s always going to be some cases where we can’t 100% emulate ActiveRecord (ie just the fact that some endpoints might not exist etc!) Without having tried it, I think for your specific use case, I would consider adding something like: def self.where(params = {})
if params[:id].is_a?(Array)
with(“recipes”).where(params)
else
super
end
end |
The straight We are looking for an API similar to AR, like documenter here: https://apidock.com/rails/ActiveRecord/FinderMethods/find. I've pushed a branch that strips the optional primary key from "standard" URI (thus generating a "collection URI"), so that the parameter is not filled in (see also the test). |
@paulvt your branch works for me. |
@johan-smits thanks, I did. Initial take was it it felt a little more...invasive? than I feel comfortable with 🙇 so I wanted to think about it a bit. Thank you for the clear spec though and the implementation proposal, but I still don't feel 100% sure if this is something that should live as a permanent part of Spyke, as it seems there are many opinions on how retrieving multiple IDs from an api should work. I guess your proposal is:
But I've also seen APIs that handle it this way:
and for example Her handles it this way:
...which kinda makes me suspect it's a decision that needs to be made on a per-API basis...? 🤔 |
Yes, I agree. The changes in my branch for Also, note that the changes lean on Spyke's current behaviour for multi-valued queries, so that |
It makes the 1,2 only when it is the primary key. If not it uses the id[].. notation. This is not consistent. |
Ah, to clarify, it's not really about being the primary key, the behavior is based on whether the parameter is a variable in the URI or not, so for example any of these (not just
In that sense I believe it's actually pretty consistent? Again, I believe you can use Spyke as a starting point upon which you add the behavior unique to your API and any methods your users require. Therefore, if you have a requirement that module Api
class Base < Spyke::Base
def self.find(ids*)
# ... define the behavior you want
end
end
class Post < Base
end
end |
The problem I have in this case is then that it not only requires and override of |
When you do a where query with the primary key in it uses does a find.
It should be possible to do for example:
Recipe.where(id: [1,2], title: 'foo')
but now it results in:http://sushi.com/recipes/1,2?title=foo
and it should be:http://sushi.com/recipes?id[]=1&id[]=2&title=foo
The text was updated successfully, but these errors were encountered: