Fix encoding issue with JSON Cache (!21407) · Merge requests · GitLab.org / GitLab

Nicolas Dular requested to merge nicolasdular/fix-json-cache-issue into master Dec 09, 2019

What does this MR do?

Initial Problem

While working on !21038 (merged) I figured out that calling BroadcastMessage.current twice gives me not the same results. Initially, broadcast_type was for example "banner" (when it was fetched from the database), but on the second call (when fetching it from JSONCache) broadcast_type resulted into nil.

Solution

While checking JSONCache I've figured out that build_from_database is not necessarily compatible with init_with for enums (see Investigation below for details). The init_with documentation states that it's guaranteed to work when used in combination with encode_with though which this MR introduces.

Investigation

For those who are interested in the details:

1. We use `to_json` for our `JSONCache` to store the data of a Model. The raw data for enums is therefore a `String`.

  {
     "id"=>1,
     "message"=>"MyText",
     "starts_at"=>"2019-12-08T11:44:03.792Z",
     "ends_at"=>"2019-12-10T11:44:03.792Z",
     "created_at"=>"2019-12-09T11:44:03.793Z",
     "updated_at"=>"2019-12-09T11:44:03.793Z",
     "color"=>"#E75E40",
     "font"=>"#FFFFFF",
     "message_html"=>"<p>MyText</p>",
     "cached_markdown_version"=>1179648,
     "target_path"=>"foo",
     "broadcast_type"=>"banner" # Rails converts enums to Strings when serialising to JSON
  }

2. When retrieving the data again, we use `init_with` and assign the `attributes` for our Model. To assign the attributes we're using `build_from_database`, which has the following structure:

klass.attributes_builder.build_from_database(raw, {})
=> #<ActiveModel::AttributeSet:0x00007fae77ac9950
 @attributes=
  #<ActiveModel::LazyAttributeHash:0x00007fae77ac99a0
   @additional_types={},
   @default_attributes=
    {"id"=>
      #<ActiveModel::Attribute::FromDatabase:0x00007faec657be68
       @name="id",
       @original_attribute=nil,
       @type=
        #<ActiveModel::Type::Integer:0x00007fae77201430 @limit=4, @precision=nil, @range=-2147483648...2147483648, @scale=nil>,
       @value_before_type_cast=nil>},
   @delegate_hash={},
   @materialized=false,
   @types=
    {"id"=>#<ActiveModel::Type::Integer:0x00007fae77201430 @limit=4, @precision=nil, @range=-2147483648...2147483648, @scale=nil>,
     "message"=>#<ActiveRecord::Type::Text:0x00007fae77200f30 @limit=nil, @precision=nil, @scale=nil>,
     "starts_at"=>
      #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::DateTime:0x00007fae7729c278 @limit=nil, @precision=nil, @scale=nil>,
     "ends_at"=>
      #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::DateTime:0x00007fae7729c278 @limit=nil, @precision=nil, @scale=nil>,
     "created_at"=>
      #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::DateTime:0x00007fae7729c278 @limit=nil, @precision=nil, @scale=nil>,
     "updated_at"=>
      #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::DateTime:0x00007fae7729c278 @limit=nil, @precision=nil, @scale=nil>,
     "color"=>#<ActiveModel::Type::String:0x00007fae7729d010 @limit=nil, @precision=nil, @scale=nil>,
     "font"=>#<ActiveModel::Type::String:0x00007fae7729d010 @limit=nil, @precision=nil, @scale=nil>,
     "message_html"=>#<ActiveRecord::Type::Text:0x00007fae77200f30 @limit=nil, @precision=nil, @scale=nil>,
     "cached_markdown_version"=>
      #<ActiveModel::Type::Integer:0x00007fae77201430 @limit=4, @precision=nil, @range=-2147483648...2147483648, @scale=nil>,
     "target_path"=>#<ActiveModel::Type::String:0x00007faec6572db8 @limit=255, @precision=nil, @scale=nil>,
     "broadcast_type"=>
      #<ActiveRecord::Enum::EnumType:0x00007faec657add8
       @mapping={"banner"=>1, "notification"=>2},
       @name="broadcast_type",
       @subtype=#<ActiveModel::Type::Integer:0x00007fae772015e8 @limit=2, @precision=nil, @range=-32768...32768, @scale=nil>>},
   @values=
    {"id"=>1,
     "message"=>"MyText",
     "starts_at"=>"2019-12-08T11:44:03.792Z",
     "ends_at"=>"2019-12-10T11:44:03.792Z",
     "created_at"=>"2019-12-09T11:44:03.793Z",
     "updated_at"=>"2019-12-09T11:44:03.793Z",
     "color"=>"#E75E40",
     "font"=>"#FFFFFF",
     "message_html"=>"<p>MyText</p>",
     "cached_markdown_version"=>1179648,
     "target_path"=>"foo",
     "broadcast_type"=>"banner"}>>

3. When applying the output from `build_from_database` to `init_with`, the value of `broadcast_type` does not get resolved correctly and result into `nil`.

Btw: It would resolve correctly if broadcast_type would be 1 instead of the string representation.

klass.allocate.init_with( "attributes" => klass.attributes_builder.build_from_database(raw, {}), "new_record" => new_record?(raw, klass))
=> #<BroadcastMessage:0x00007fae803c16b8
 id: 1,
 message: "MyText",
 starts_at: Sun, 08 Dec 2019 11:44:03 UTC +00:00,
 ends_at: Tue, 10 Dec 2019 11:44:03 UTC +00:00,
 created_at: Mon, 09 Dec 2019 11:44:03 UTC +00:00,
 updated_at: Mon, 09 Dec 2019 11:44:03 UTC +00:00,
 color: "#E75E40",
 font: "#FFFFFF",
 message_html: "<p>MyText</p>",
 cached_markdown_version: 1179648,
 target_path: "foo",
 broadcast_type: nil>

4. The structure of `encode_with` is a bit different and is also guaranteed to work with `init_with` and fixes the issue:

BroadcastMessage.new(raw_json).encode_with(coder)
coder
=> {"concise_attributes"=>
  [#<ActiveModel::Attribute::FromUser:0x00007fd0b18ebb90
    @name="id",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017f40
      @name="id",
      @original_attribute=nil,
      @type=#<ActiveModel::Type::Integer:0x00007fd0b6c85e90 @limit=4, @precision=nil, @range=-2147483648...2147483648, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast=1>,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18ebb40
    @name="message",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017ea0
      @name="message",
      @original_attribute=nil,
      @type=#<ActiveRecord::Type::Text:0x00007fd0b6c85990 @limit=nil, @precision=nil, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast="MyText">,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18ebaa0
    @name="starts_at",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017e50
      @name="starts_at",
      @original_attribute=nil,
      @type=
       #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::DateTime:0x00007fd0b6cdce70 @limit=nil, @precision=nil, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast="2019-12-08T13:26:16.368Z">,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb9b0
    @name="ends_at",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017db0
      @name="ends_at",
      @original_attribute=nil,
      @type=
       #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::DateTime:0x00007fd0b6cdce70 @limit=nil, @precision=nil, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast="2019-12-10T13:26:16.368Z">,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb910
    @name="created_at",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017ce8
      @name="created_at",
      @original_attribute=nil,
      @type=
       #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::DateTime:0x00007fd0b6cdce70 @limit=nil, @precision=nil, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast="2019-12-09T13:26:16.368Z">,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb870
    @name="updated_at",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017c48
      @name="updated_at",
      @original_attribute=nil,
      @type=
       #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::DateTime:0x00007fd0b6cdce70 @limit=nil, @precision=nil, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast="2019-12-09T13:26:16.368Z">,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb7d0
    @name="color",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2016028
      @name="color",
      @original_attribute=nil,
      @type=#<ActiveModel::Type::String:0x00007fd0b6cddb18 @limit=nil, @precision=nil, @scale=nil>,
      @value="#E75E40",
      @value_before_type_cast="#E75E40">,
    @type=nil,
    @value_before_type_cast="#E75E40">,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb758
    @name="font",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2015330
      @name="font",
      @original_attribute=nil,
      @type=#<ActiveModel::Type::String:0x00007fd0b6cddb18 @limit=nil, @precision=nil, @scale=nil>,
      @value="#FFFFFF",
      @value_before_type_cast="#FFFFFF">,
    @type=nil,
    @value_before_type_cast="#FFFFFF">,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb6b8
    @name="message_html",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017ba8
      @name="message_html",
      @original_attribute=nil,
      @type=#<ActiveRecord::Type::Text:0x00007fd0b6c85990 @limit=nil, @precision=nil, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast="<p>MyText</p>">,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb690
    @name="cached_markdown_version",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017ab8
      @name="cached_markdown_version",
      @original_attribute=nil,
      @type=#<ActiveModel::Type::Integer:0x00007fd0b6c85e90 @limit=4, @precision=nil, @range=-2147483648...2147483648, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast=1179648>,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb5a0
    @name="target_path",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017a90
      @name="target_path",
      @original_attribute=nil,
      @type=#<ActiveModel::Type::String:0x00007fd0b10293b8 @limit=255, @precision=nil, @scale=nil>,
      @value=nil,
      @value_before_type_cast=nil>,
    @type=nil,
    @value_before_type_cast=nil>,
   #<ActiveModel::Attribute::FromUser:0x00007fd0b18eb438
    @name="broadcast_type",
    @original_attribute=
     #<ActiveModel::Attribute::FromDatabase:0x00007fd0b2017a40
      @name="broadcast_type",
      @original_attribute=nil,
      @type=
       #<ActiveRecord::Enum::EnumType:0x00007fd0b104b030
        @mapping={"banner"=>1, "notification"=>2},
        @name="broadcast_type",
        @subtype=#<ActiveModel::Type::Integer:0x00007fd0b6c85fd0 @limit=2, @precision=nil, @range=-32768...32768, @scale=nil>>,
      @value="banner",
      @value_before_type_cast="1">,
    @type=nil,
    @value_before_type_cast="banner">],
 "new_record"=>true,
 "active_record_yaml_version"=>2

# the resulting broadcast message
#<BroadcastMessage:0x00007fdf157b8008
 id: 1,
 message: "MyText",
 starts_at: Mon, 09 Dec 2019 12:24:28 UTC +00:00,
 ends_at: Wed, 11 Dec 2019 12:24:28 UTC +00:00,
 created_at: Tue, 10 Dec 2019 12:24:28 UTC +00:00,
 updated_at: Tue, 10 Dec 2019 12:24:28 UTC +00:00,
 color: "#E75E40",
 font: "#FFFFFF",
 message_html: "<p>MyText</p>",
 cached_markdown_version: 1179648,
 target_path: nil,
 broadcast_type: "banner">

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
Tested in all supported browsers

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

Label as security and @ mention @gitlab-com/gl-security/appsec
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
Security reports checked/validated by a reviewer from the AppSec team

Edited Dec 10, 2019 by Nicolas Dular

Fix encoding issue with JSON Cache