rubyforgood / Flaredown

Flaredown web app and API
http://www.flaredown.com
GNU General Public License v3.0
39 stars 15 forks source link

Namespace serializers for consuming controller #538

Closed benlangfeld closed 3 years ago

benlangfeld commented 3 years ago

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

bklang commented 3 years ago

Great investigation, awesome documentation on the findings!