search

okuramasafumi / alba

Alba is a JSON serializer for Ruby, JRuby and TruffleRuby.
https://okuramasafumi.github.io/alba/
MIT License
934 stars 43 forks source link

You passed "location" options but ignored. #347

Closed mehanoid closed 10 months ago

mehanoid commented 10 months ago

Describe the bug

When using the location option in the render method, a warning is shown

To Reproduce

render json: FooResource.new(foo_instance), location: foo_instance

Expected behavior

No warnings

Actual behavior

The following message is shown:

You passed "location" options but ignored. Please refer to the document: https://github.com/okuramasafumi/alba/blob/main/docs/rails.md

Environment

Additional context

location is a valid option for the render method. In the code there is a whitelist of options for which alba does not show warnings. Among them there is no location option. I think that at least the location option should be added to the whitelist too.

Although perhaps it would be better to blacklist the options for which the warning will be shown? As I understand it, the warning was originally added to avoid confusion with the only or except options, which can be mistakenly added, expecting that only the specified attributes will be shown in the server response.

At the same time, it is arguable whether any warnings are necessary at all. Rails itself does not do any checks for invalid options. Should alba take over this responsibility?

okuramasafumi commented 10 months ago

@mehanoid Thank you for pointing out. Yes, location is a valid option for render so this should be fixed. The problem is how to fix it. As you mentioned only and except are confusing but others are not, so we can warn only about those two options. I believe this warning itself is valuable since some people will expect only and except work and they submit bug reports because they don't work.

mehanoid commented 10 months ago

I think warning about these two options would be much better than the current implementation anyway. It would also be more resilient to possible future changes in the rails api.

okuramasafumi commented 10 months ago

@mehanoid I noticed that methods option could be also confusing. What do you think about it?

mehanoid commented 10 months ago

It seems to me that we should be guided by the potential usecases and the result someone might expect when using these options. The only and except options would obviously be expected to render only the specified attributes. But what would be expected when passing the methods option? That the specified methods would be called on the Resource instance? That seems a bit odd.

okuramasafumi commented 10 months ago

Makes sense. I'll start with only and except only, and when people think we should add methods to that list, I'll do so.