Skip to content

Conversation

@arielj
Copy link

@arielj arielj commented Oct 18, 2023

This PR includes different changes:

  • use strong parameters to permit/require parameters in the requests
  • use AR validations to require attributes in the report model
  • updates tests to send json content since it matches the information we want better
  • use AR serialization to handle the "report" column de/serialization

@arielj arielj requested a review from etagwerker October 18, 2023 20:07
@arielj arielj linked an issue Oct 18, 2023 that may be closed by this pull request
2 tasks
@etagwerker etagwerker force-pushed the Issue-14-more-conventional-rails branch from 83f4e4a to 406644a Compare October 24, 2025 19:08
Copy link
Member

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arielj Sorry for the delay!

This looks good, but I'm getting an error when testing it locally:

Started POST "/reports" for ::1 at 2025-10-24 16:34:42 -0400
  ActiveRecord::SchemaMigration Load (0.5ms)  SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
Processing by ReportsController#create as */*
  Parameters: {"{\"entries\":"=>{"{\"name\":\"Enumerable#reverse.detect\",\"central_tendency\":191701.4517993972,\"ips\":191701.4517993972,\"error\":5033,\"stddev\":5033,\"microseconds\":5043669.0,\"iterations\":966160,\"cycles\":18580},{\"name\":\"Enumerable#select.last\",\"central_tendency\":5433.2019560685785,\"ips\":5433.2019560685785,\"error\":88,\"stddev\":88,\"microseconds\":5044405.0,\"iterations\":27400,\"cycles\":548}"=>{",\"options\":{\"compare\":true}}"=>nil}}}
Completed 400 Bad Request in 4ms (ActiveRecord: 0.0ms (0 queries, 0 cached) | GC: 0.2ms)

What's concerning is that the test suite passes (when it probably shouldn't)

The same request works in main but it fails in this branch. Thoughts?

@arielj
Copy link
Author

arielj commented Oct 28, 2025

@arielj Sorry for the delay!

This looks good, but I'm getting an error when testing it locally:

Started POST "/reports" for ::1 at 2025-10-24 16:34:42 -0400
  ActiveRecord::SchemaMigration Load (0.5ms)  SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
Processing by ReportsController#create as */*
  Parameters: {"{\"entries\":"=>{"{\"name\":\"Enumerable#reverse.detect\",\"central_tendency\":191701.4517993972,\"ips\":191701.4517993972,\"error\":5033,\"stddev\":5033,\"microseconds\":5043669.0,\"iterations\":966160,\"cycles\":18580},{\"name\":\"Enumerable#select.last\",\"central_tendency\":5433.2019560685785,\"ips\":5433.2019560685785,\"error\":88,\"stddev\":88,\"microseconds\":5044405.0,\"iterations\":27400,\"cycles\":548}"=>{",\"options\":{\"compare\":true}}"=>nil}}}
Completed 400 Bad Request in 4ms (ActiveRecord: 0.0ms (0 queries, 0 cached) | GC: 0.2ms)

What's concerning is that the test suite passes (when it probably shouldn't)

The same request works in main but it fails in this branch. Thoughts?

it looks like benchmark-ips is sending the request with no content_type, so rails parses the params incorrectly and ends up with "{\"entries\":" as the key instead of :entries or "entries".

params.keys
# => ["{\"entries\":", "controller", "action"]

I updated the code adding a test for this scenario and also code in the controller that updates the params object with the body correctly parsed in these cases

for completeness, if we add req.content_type = "application/json" here https://github.com/evanphx/benchmark-ips/blob/38e330e6323b16ab3ab09fd37ad3792017ac8a9b/lib/benchmark/ips/share.rb#L29 it does send the request with the correct content_type and rails parses it correctly, but we'd still need this new code here to handle request made by older versions of the gem anyway

@etagwerker
Copy link
Member

for completeness, if we add req.content_type = "application/json" here https://github.com/evanphx/benchmark-ips/blob/38e330e6323b16ab3ab09fd37ad3792017ac8a9b/lib/benchmark/ips/share.rb#L29 it does send the request with the correct content_type and rails parses it correctly, but we'd still need this new code here to handle request made by older versions of the gem anyway

@arielj This would be great. Could you please open a PR for that in the benchmark-ips project?

@etagwerker etagwerker merged commit cb33fd1 into main Oct 29, 2025
1 of 2 checks passed
@etagwerker etagwerker deleted the Issue-14-more-conventional-rails branch October 29, 2025 14:31
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.

[REQUEST] Make reports#create more standard

3 participants