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.

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

Namespace serializers for consuming controller #538

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

benlangfeld
Copy link
Contributor

@benlangfeld benlangfeld commented Oct 14, 2021

This change moves all serializers into the same namespace (Api::V1) as the controllers which consume those serializers. In doing so it fixes a bug which currently breaks almost any operation in staging, most notably user signup, by falling back to Object#as_json to serialize all API responses, not complying at all with our API spec.

active_model_serializers looks for the serializers in this namespace by default, but this worked on Ruby 2.6.5:

[1] pry(#<Api::V1::RegistrationsController>)> r = Registration.create!(params)=> #<Registration:0x00007f96688f4f20
 @birth_date=nil,
 @captcha_response=
  "dummy-captcha-response",
 @errors=
  #<ActiveModel::Errors:0x00007f96688f4188
   @base=
    #<Registration:0x00007f96688f4f20 ...>,
   @errors=[]>,
 @screen_name="user",
 @user=
  #<User id: 1, email: "user@example.com", authentication_token: "03441d861500b01606fa928521b971eb", created_at: "2021-10-14 21:14:00.673552000 +0000", updated_at: "2021-10-14 21:14:00.673552000 +0000">,
 @user_params=
  {"email"=>"user@example.com",
   "password"=>"secret123",
   "password_confirmation"=>"secret123"}>
[2] pry(#<Api::V1::RegistrationsController>)> default_serializer(r)=> RegistrationSerializer

Since Ruby 2.6.6, a change to constant lookup was introduced which means that the appropriate class can't be found:

[2] pry(#<Api::V1::RegistrationsController>)> default_serializer(r)
=> nil
[3] pry(#<Api::V1::RegistrationsController>)> ActiveModel::Serializer.serializer_for(r, namespace: namespace_for_serializer)
=> nil

The reason is that while in both Ruby versions, the constant name that we try to look up is namespaced (This namespacing behaviour is documented in later releases, but the behaviour is similar in v0.9.x.):

[4] pry(#<Api::V1::RegistrationsController>)> ActiveModel::Serializer.send(:build_serializer_class, r, namespace: namespace_for_serializer)
=> "Api::V1::RegistrationSerializer"

this name is passed to Object#const_get which behaves more strictly since Ruby 2.6.6:

Ruby 2.6.5:

[5] pry(#<Api::V1::RegistrationsController>)> Object.const_get "Api::V1::RegistrationSerializer"
=> RegistrationSerializer

Ruby 2.6.6:

[5] pry(#<Api::V1::RegistrationsController>)> Object.const_get "Api::V1::RegistrationSerializer"
=> nil

It is considered appropriate to namespace the serializers together with the consuming controllers because together they constitute a cohesive definition of an API version; should an API V2 be introduced, it should likely have distinct serializers.

Interesting source material:

Bug was introduced in #470

Screenshot 2021-10-14 at 19 19 45

This change moves all serializers into the same namespace (`Api::V1`) as the controllers which consume those serializers. In doing so it fixes a bug which currently breaks almost any operation in staging, most notably user signup, by falling back to `Object#as_json` to serialize all API responses, not complying at all with our API spec.

`active_model_serializers` [looks for the serializers in this namespace by default](https://github.com/rails-api/active_model_serializers/blob/v0.9.8/lib/action_controller/serialization.rb#L62-L80), but this worked on Ruby 2.6.5:

```
[1] pry(#<Api::V1::RegistrationsController>)> r = Registration.create!(params)=> #<Registration:0x00007f96688f4f20
 @birth_date=nil,
 @captcha_response=
  "dummy-captcha-response",
 @errors=
  #<ActiveModel::Errors:0x00007f96688f4188
   @base=
    #<Registration:0x00007f96688f4f20 ...>,
   @errors=[]>,
 @screen_name="user",
 @user=
  #<User id: 1, email: "user@example.com", authentication_token: "03441d861500b01606fa928521b971eb", created_at: "2021-10-14 21:14:00.673552000 +0000", updated_at: "2021-10-14 21:14:00.673552000 +0000">,
 @user_params=
  {"email"=>"user@example.com",
   "password"=>"secret123",
   "password_confirmation"=>"secret123"}>
[2] pry(#<Api::V1::RegistrationsController>)> default_serializer(r)=> RegistrationSerializer
```

Since Ruby 2.6.6, [a change to constant lookup](ruby/ruby@15eba4e) was introduced which means that the appropriate class can't be found:

```
[2] pry(#<Api::V1::RegistrationsController>)> default_serializer(r)
=> nil
[3] pry(#<Api::V1::RegistrationsController>)> ActiveModel::Serializer.serializer_for(r, namespace: namespace_for_serializer)
=> nil
```

The reason is that while in both Ruby versions, the constant name that we try to look up is namespaced ([This namespacing behaviour is documented](https://github.com/rails-api/active_model_serializers/blob/v0.10.6/docs/general/rendering.md#namespace) in later releases, but the behaviour is similar in v0.9.x.):

```
[4] pry(#<Api::V1::RegistrationsController>)> ActiveModel::Serializer.send(:build_serializer_class, r, namespace: namespace_for_serializer)
=> "Api::V1::RegistrationSerializer"
```

this name [is passed to Object#const_get](https://github.com/rails-api/active_model_serializers/blob/ff1a36ffbc82c6070905707b29978a7b897dea81/lib/active_model/serializable/utils.rb#L6) which behaves more strictly since Ruby 2.6.6:

Ruby 2.6.5:

```
[5] pry(#<Api::V1::RegistrationsController>)> Object.const_get "Api::V1::RegistrationSerializer"
=> RegistrationSerializer
```

Ruby 2.6.6:

```
[5] pry(#<Api::V1::RegistrationsController>)> Object.const_get "Api::V1::RegistrationSerializer"
=> nil
```

It is considered appropriate to namespace the serializers together with the consuming controllers because together they constitute a cohesive definition of an API version; should an API V2 be introduced, it should likely have distinct serializers.
@bklang
Copy link
Collaborator

bklang commented Oct 14, 2021

Great investigation, awesome documentation on the findings!

@benlangfeld benlangfeld added the type:bug Issues that impair or prevent product functionality label Oct 14, 2021
@benlangfeld benlangfeld marked this pull request as ready for review October 14, 2021 22:20
backend/spec/spec_helper.rb Outdated Show resolved Hide resolved
@benlangfeld benlangfeld enabled auto-merge (squash) October 14, 2021 22:28
@benlangfeld benlangfeld merged commit 25976d1 into master Oct 14, 2021
@benlangfeld benlangfeld deleted the 20211014-serializer-namespacing branch October 14, 2021 22:29
@bklang bklang added type:bug Issues that impair or prevent product functionality Hacktoberfest hacktoberfest-accepted hacktober-fest and removed type:bug Issues that impair or prevent product functionality labels Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest hacktoberfest-accepted type:bug Issues that impair or prevent product functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants