With rails 7.1, these nil values are not persisted #884
Conversation
|
Side q - I'm curious why some gems downgraded like the ibm_vpc ones |
let me rebase my rails71 branch and see if it cleans up some of those changes. |
| "has_default_path" => false, | ||
| "short_description" => "EmsRefreshSpecProductWithNoPortfolio desc", | ||
| "support_description" => nil |
There was a problem hiding this comment.
Note, if the cassette has these values, the test passes fine with rails 7.1, see line 1470 below...
"support_url" => "http://EmsRefreshSpec.com",
"support_email" => "[email protected]",
"has_default_path" => false,
"short_description" => "EmsRefreshSpecProduct description",
"support_description" => "EmsRefreshSpec desc"There was a problem hiding this comment.
In other words, if the cassette has this information, it's populated, if the keys don't exist in the cassette with rails 7.1, the key doesn't even exist.
There was a problem hiding this comment.
@agrare thoughts on this removal? I don't know why these values are populated as nil with rails 7.0 if missing from the cassette and not specified at all with rails 7.1. 🤷
There was a problem hiding this comment.
I wonder if there is a difference in the record being saved or if the difference is in the matcher picking up on the nested hash value? Let me try running this on rails 7.0 and dump that property for each iteration and see what we get.
There was a problem hiding this comment.
So we don't set those keys specifically we just take the whole product_view_summary from AWS
:extra => {
:product_view_summary => service_offering.product_view_summary,I did throw in a expect(@service_offering_with_no_portfolio.extra["product_view_summary"]).to have_key("support_description") to check that the key always existed and wasn't just missing and that passed on 7.0.
I'll try pulling down your 7.1 branch and see if I can spot the difference (I wonder if setting a struct Aws::ServiceCatalog::Types::ProductViewSummary to a jsonb field is being handled differently in 7.1?)
There was a problem hiding this comment.
Can you try the following on that struct in each version? This is what's used via the ActiveRecord::Type::Json class. Ultimately, I it calls as_json under the covers.
pp value
e = ActiveSupport::JSON.encode(value)
pp e
d = ActiveSupport::JSON.decode(e)
pp dThere was a problem hiding this comment.
Actually, I just tried it myself, and it's definitely the as_json method changed:
Taking our app out of the equation, I wrote this script
require "bundler/inline"
gemfile do
gem "aws-sdk-servicecatalog"
gem "activesupport", ARGV[0], :require => "active_support/all"
end
puts "Using ActiveSupport #{ActiveSupport.version}"
puts
pvs = Aws::ServiceCatalog::Types::ProductViewSummary.new(
:id => "prodview-mojlvmp5xax74",
:product_id => "prod-4v6rc4hwaiiha",
:name => "EmsRefreshSpecProductWithNoPortfolio",
:owner => "EmsRefreshSpecProductWithNoPortfolioOwner",
:short_description => "EmsRefreshSpecProductWithNoPortfolio desc",
:type => "CLOUD_FORMATION_TEMPLATE",
:distributor => "",
:has_default_path => false,
:support_email => nil,
:support_description => nil,
:support_url => nil
)
pp pvs.as_jsonHere's the results on different rails versions:
$ ruby struct_as_json.rb "~>7.0.8"
Using ActiveSupport 7.0.8.7
{"id"=>"prodview-mojlvmp5xax74",
"product_id"=>"prod-4v6rc4hwaiiha",
"name"=>"EmsRefreshSpecProductWithNoPortfolio",
"owner"=>"EmsRefreshSpecProductWithNoPortfolioOwner",
"short_description"=>"EmsRefreshSpecProductWithNoPortfolio desc",
"type"=>"CLOUD_FORMATION_TEMPLATE",
"distributor"=>"",
"has_default_path"=>false,
"support_email"=>nil,
"support_description"=>nil,
"support_url"=>nil}
$ ruby struct_as_json.rb "~>7.1.5"
Using ActiveSupport 7.1.5.1
{"id"=>"prodview-mojlvmp5xax74",
"product_id"=>"prod-4v6rc4hwaiiha",
"name"=>"EmsRefreshSpecProductWithNoPortfolio",
"owner"=>"EmsRefreshSpecProductWithNoPortfolioOwner",
"short_description"=>"EmsRefreshSpecProductWithNoPortfolio desc",
"type"=>"CLOUD_FORMATION_TEMPLATE",
"distributor"=>"",
"has_default_path"=>false}
$ ruby struct_as_json.rb "~>7.2.2"
Using ActiveSupport 7.2.2.1
{"id"=>"prodview-mojlvmp5xax74",
"product_id"=>"prod-4v6rc4hwaiiha",
"name"=>"EmsRefreshSpecProductWithNoPortfolio",
"owner"=>"EmsRefreshSpecProductWithNoPortfolioOwner",
"short_description"=>"EmsRefreshSpecProductWithNoPortfolio desc",
"type"=>"CLOUD_FORMATION_TEMPLATE",
"distributor"=>"",
"has_default_path"=>false}
$ ruby struct_as_json.rb "~>8.0.1"
Using ActiveSupport 8.0.1
{"id"=>"prodview-mojlvmp5xax74",
"product_id"=>"prod-4v6rc4hwaiiha",
"name"=>"EmsRefreshSpecProductWithNoPortfolio",
"owner"=>"EmsRefreshSpecProductWithNoPortfolioOwner",
"short_description"=>"EmsRefreshSpecProductWithNoPortfolio desc",
"type"=>"CLOUD_FORMATION_TEMPLATE",
"distributor"=>"",
"has_default_path"=>false}
There was a problem hiding this comment.
@agrare @jrafanie I narrowed it down to 7.1.0.beta1, and so I'm pretty sure it's this change that landed in Rails 7.1.0.beta1: rails/rails@a7d4d78
It's weird, but that leads to rails/rails#47273, and if you follow that thread you will see fixes to the Data object for empty fields. I think this is a bug in Rails that Structs deserialize without their nil fields.
There was a problem hiding this comment.
Oh! even better. So, with that change above, as_json calls .to_h, But in aws-sdk-core their struct#to_h method removes nils by default
There was a problem hiding this comment.
Smaller reproducer:
require "bundler/inline"
gemfile do
gem "aws-sdk-core"
gem "activesupport", ARGV[0]
end
require "logger" # LOL
require "active_support"
require "active_support/core_ext/object/json"
puts "Using ActiveSupport #{ActiveSupport.version}"
MyStruct = Struct.new(:not_missing, :missing) do
include Aws::Structure # This is what aws-sdk gems do with their structures, and it brings in an alternative to_h
end
s = MyStruct.new(:not_missing => "foo", :missing => nil)
puts "as_json: #{s.as_json}"
puts "to_h: #{s.to_h}"$ ruby struct_as_json.rb "=7.0.8.7"
Using ActiveSupport 7.0.8.7
as_json: {"not_missing"=>"foo", "missing"=>nil}
to_h: {:not_missing=>"foo"}
$ ruby struct_as_json.rb "=7.1.0.beta1"
Using ActiveSupport 7.1.0.beta1
as_json: {"not_missing"=>"foo"}
to_h: {:not_missing=>"foo"}
Ok, rebased and updated the Gemfile.lock differences. I'll look at other errors and come back to this later. I'm not really sure what's happening here. |
|
In chat, I mentioned it will could be related to this: ManageIQ/manageiq#23275 |
support_url, support_email, and support_description don't exist in the response payload
in the cassette for these tests and yet previously, these fields were set as nil with rails
7.0. It's unclear what changed with rails 7.1 but now these nil value fields aren't persisted
in the extra column.
collector/cloud_manager.rb:
def service_offerings
aws_service_catalog.client.search_products_as_admin.product_view_details
...
end
parser/cloud_manager.rb:
collector.service_offerings.each do |service_offering|
persister_service_offering = persister.service_offerings.build(...
...
:extra => {
:product_view_summary => service_offering.product_view_summary,
...
Prevent differing behavior between rails versions when assigning a Struct to a jsonb object by explicitly converting to a hash first.
|
I tried reverting the change to the newer config gem to continue to use deep_merge from the gem and not active_support and i saw the same behavior on rails 7.1. I attempted to bisect v7.0 .. v7.1 but there were too many issues that were fixed in newer versions that prevented me from testing the merge base between the two. The issue is definitely how the struct is persisted to a jsonb column, in order to behave consistently between 7.0 and 7.1 I updated the parser to call to_h on the struct before assigning it to the InventoryObject to be persisted. I have a commit in this branch now so assigning to @Fryguy |
agrare
changed the title
|
Merging this PR as it works on both Rails 7.0 and 7.1. tl;dr Rails jsonb column encoding uses as_json; as_json was changed in activesupport 7.1.0.beta1 to call to_h; aws-sdk structs have a custom to_h that removes nil values. We might need to evaluate separately if other structs should be |
| @@ -404,7 +404,7 @@ def service_offerings | |||
| :name => service_offering.product_view_summary.name, | |||
| :ems_ref => service_offering.product_view_summary.product_id, | |||
| :extra => { | |||
| :product_view_summary => service_offering.product_view_summary, | |||
| :product_view_summary => service_offering.product_view_summary.to_h, | |||
UPDATE: @agrare dug in and nerd sniped @Fryguy who figured it out... Struct#as_json changed with rails 7.1 see the discussion.
We now just convert to hash to avoid the change in Struct#as_json in rails 7.1.
Old Description:
support_url, support_email, and support_description don't exist in the response payload in the cassette for these tests, while others have it, and yet previously, these unset fields were set as nil with rails 7.0. It's unclear what changed with rails 7.1 but now these nil value fields aren't persisted in the extra column, so I had to remove them in the tests where these values in the cassettes didn't exist in the response.
collector/cloud_manager.rb:
parser/cloud_manager.rb:
Here is my Gemfile.lock with rails 7.0 vs. rails 7.1, using the same aws-sdk gems: