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

Issue 120 Fix bug cannot create Tag Definitions #386

Merged
merged 1 commit into from
Nov 26, 2023

Conversation

tungleduyxyz
Copy link
Contributor

@tungleduyxyz tungleduyxyz commented Nov 26, 2023

The issue raised here: rack/rack#2128
I found that the issue comes from the rack gem, the parse_nested_query function.
The Gemfile from killbill-admin-ui: gem 'rails', '~> 7.0.1' -> rails 7.0.8 -> will install rack 2.2.8
The Gemfile from killbill-admin-ui-standalone: gem 'rails', '~> 7.0.0' -> rails 7.1.2 -> will install rack 3.0.8
You can compare the parse_nested_queryhere:

Here is our raw source data for POST:

authenticity_token=1cC60RSAumGOde_9icOPLNwLOUroWsWAb7mmqI6S6McpehjcvoKr8p57qivQd_F3DyHjrcGyksSXaq2EWrPEhg&t
ag_definition%5Bid%5D=&tag_definition%5Bapplicable_object_types%5B0%5D%5D=SUBSCRIPTION&tag_definition%5Bname%5D=test1&tag_definition%5Bdescription%5D=te
st1&commit=Save

And the result when using Rack::Utils.parse_nested_query:

  • 3.0.8:
{"authenticity_token"=>"1cC60RSAumGOde_9icOPLNwLOUroWsWAb7mmqI6S6McpehjcvoKr8p57qivQd_F3DyHjrcGyksSXaq2EWrPEhg",
 "tag_definition"=>{"id"=>"", "applicable_object_types[0"=>{"]"=>"SUBSCRIPTION"}, "name"=>"test1", "description"=>"test1"},
 "commit"=>"Save"}
  • 2.2.8:
{"authenticity_token"=>"1cC60RSAumGOde_9icOPLNwLOUroWsWAb7mmqI6S6McpehjcvoKr8p57qivQd_F3DyHjrcGyksSXaq2EWrPEhg",
 "t\nag_definition"=>{"id"=>""},
 "tag_definition"=>{"applicable_object_types"=>{"0"=>"SUBSCRIPTION"}, "name"=>"test1", "description"=>"te\nst1"},
 "commit"=>"Save"}

Rack 3.0 no longer supports parsing nested params like a[b[c]], the correct nested params should be a[b][c].

@pierre pierre merged commit fc9902d into killbill:master Nov 26, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants