Skip to content
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.

Sign up for GitHub

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

Allow users to globally opt out of automatic lookup #1295

Closed
wants to merge 1 commit into from
Closed

Allow users to globally opt out of automatic lookup #1295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/general/configuration_options.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ The following configuration options can be set on `ActiveModel::Serializer.confi
## General

- `adapter`: The [adapter](adapters.md) to use. Possible values: `:attributes, :json, :json_api`. Default: `:attributes`.
- `automatic_lookup`: Whether serializer should be automatically looked up or manually provided. Default: `true`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trek (hi!)

actually, you can pass adapter: false in your render (or options[:adapter] = false if you're outside a controller) and it won't look up a serializer or use an adapter.

ref:

  • active_model_serializers/lib/active_model/serializable_resource.rb

    Lines 54 to 58 in f3403c3

    # True when no explicit adapter given, or explicit appear is truthy (non-nil)
    # False when explicit adapter is falsy (nil or false)
    def use_adapter?
    !(adapter_opts.key?(:adapter) && !adapter_opts[:adapter])
    end

  • active_model_serializers/lib/action_controller/serialization.rb

    Lines 23 to 40 in f3403c3

    if !use_adapter?
    warn 'ActionController::Serialization#use_adapter? has been removed. '\
    "Please pass 'adapter: false' or see ActiveSupport::SerializableResource.new"
    options[:adapter] = false
    end
    serializable_resource = ActiveModel::SerializableResource.new(resource, options)
    if serializable_resource.serializer?
    serializable_resource.serialization_scope ||= serialization_scope
    serializable_resource.serialization_scope_name = _serialization_scope
    begin
    serializable_resource.adapter
    rescue ActiveModel::Serializer::CollectionSerializer::NoSerializerError
    resource
    end
    else
    resource
    end
    end

That should work for you, and would still be useful to include in this PR, to call that out :)

It should also be mentioned in https://github.com/rails-api/active_model_serializers/blob/master/docs/ARCHITECTURE.md, I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's not sufficient, that's the area of the public API that should be affected.. would love to see a code example of what you're trying to do

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, #1294 was merged, does that address this issue or should we revert it and continue here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear if that PR addresses the issue here.
My feeling is that it may be a usage issue (the reason for this PR), but we need more information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4 (hey!)

#1294 addresses 1 of the 2 problem cases. I think the root problem is that automatic lookup is too aggressive of a strategy for larger apps that want to introduce AMS only for new versions of an existing API. It'd be nice – and frankly safer – to opt out globally and opt-in locally in these cases.

Another riff on this PR is to make that an explicit flag on render calls:

render json: photos, use_serializer: true

But I already personally prefer supplying the specific serializer to the render call. Yes, it's repetitive, but removes the "where the heck is this object turning into this JSON?!?" problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now AMS automatically includes the ActionController::Serialization mixin. What if you could opt out of that and mix it in to controllers as desired. Would that work? (As opposed to passing in adapter: false in most of your controllers)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4 are you think that could be an initialization configuration option? something like:

include_action_controller_serialization = true (by default)

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trek re:

render json: photos, use_serializer: true

In rails/rails#21496 I have proposed removing use_renderers since it appears never to have been used or tested. Since your suggestion here has a similar name, I was wondering if it might be related to the use_renderers method in the Rails Renderer, or just coincidental


## JSON API

Expand Down
4 changes: 4 additions & 0 deletions lib/action_controller/serialization.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ def serialization_scope
end

def get_serializer(resource, options = {})
unless options[:serializer] || options[:each_serializer] || ActiveModel::Serializer.config.automatic_lookup
return resource
end

if !use_adapter?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you could also make an argument to bring back use_adapter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or add ActiveModel::Serializer.config.automatic_lookup as config.implicit_serializers = false which would used probably in the serializable resource

warn 'ActionController::Serialization#use_adapter? has been removed. '\
"Please pass 'adapter: false' or see ActiveSupport::SerializableResource.new"
Expand Down
1 change: 1 addition & 0 deletions lib/active_model/serializer/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ def config.array_serializer

config.adapter = :attributes
config.jsonapi_resource_type = :plural
config.automatic_lookup = true
end
end
end
Expand Down