Bad results with Rails #2

jscheid · 2023-05-02T11:26:20Z

Describe the bug

Results don't always match the spec when using this gem with also the rails gem loaded (more precisely: the activesupport gem, which is a dependency of rails).

This is because Rails overrides a number of to_json methods and modifies them in a way that is incompatible, for example.

To reproduce
Steps to reproduce the behavior:

Install and require this gem and rails
"&".to_json_c14n should give "&" but instead gives "\u0026".

Expected behavior
This gem's behavior shouldn't depend on the presence of other gems, at least not ones as common as Rails.

The text was updated successfully, but these errors were encountered:

jscheid · 2023-05-02T11:29:21Z

I believe the best fix here to be using ::JSON.generate(value) instead of value.to_json everywhere.

Closes dryruby#2

Closes #2

gkellogg · 2023-05-02T14:26:35Z

Thanks for the PR. I’ll release an update shortly.

jscheid · 2023-05-02T15:52:17Z

Thanks, appreciate it.

sumirolabs · 2023-12-01T18:15:17Z

This should have been a breaking change. We have apps that use this gem on both sides of an API call to generate signatures and when one (and only one) side uses v 0.3.2 the signatures do not match.

This seems like a good / proper update, but since the output has changed, I think this should have been flagged as a "breaking" change.

gkellogg · 2023-12-01T20:16:25Z

Can't quite see how the change from #to_json to ::JSON.generate() would be a breaking change. Is there some remediation you'd like to see? Do you have a test case that can be added to illustrate the issue?

sumirolabs · 2023-12-04T02:22:45Z

As I understand this PR (and read the code), the goal was remove the changes that Rails may inject into how JSON is generated. This means that the previous versions of this gem could produce different output.

I'm thinking that because the output changed, the gem is no longer behaving exactly the same as before and thus any app that depended upon the previous behavior could now be "broken".

I realize that this could also be considered a bugfix, but identifying this change as significant could have helped us detect that this new version of the gem is not compatible with older versions when used to generate signatures in our apps.

gkellogg · 2023-12-04T02:35:39Z

Well, we can always release the same code as 3.3.0 and pull the current version from RybyGems, if that would help.

my guess is that you are depending on something else that overloads #to_json that resulted in this dependency. You may be canonicalizing data that is not reduced to simple maps, arrays, numbers, strings and booleans.

sumirolabs · 2023-12-04T16:01:44Z

my guess is that you are depending on something else that overloads #to_json that resulted in this dependency.

Yes, we are using this in a Rails app that overloads to_json, which is why this PR was made.

Anytime someone uses v0.3.2 and a previous version of json-canonicalization (in 2 separate apps), the output will be different because 0.3.2 will not include the Rails overrides while < 0.3.2 will include the Rails overrides - or am I missing something here?

My suggestion is to bump the version to 1.0.0 and add a CHANGELOG.md that describes how 0.3.2 / 1.0.0 differs from < 0.3.2.

gkellogg · 2023-12-04T20:10:40Z

I'll release a 1.0.0 based on the current 0.3.2. But, note that yanking 0.3.2 may break some dependencies as well.

We use the GitHub release mechanism in preference to a CHANGELOG.md file.

gkellogg · 2023-12-04T20:14:27Z

0.3.2 yanked and 1.0.0 released.

renchap · 2023-12-05T08:30:58Z

You should not have yanked the 0.3.0 version, this is breaking software depending on this version.

For example, Mastodon (mastodon/mastodon#28224). This requires us to release a new version as nobody can install a stable version of Mastodon 4.2 (4.2.0, 4.2.1 or 4.2.2) anymore.

Would it be possible to restore version 0.3.2?

sumirolabs · 2023-12-05T12:00:28Z

While I appreciate the version bump, I think yanking 0.3.2 probably wasn't the best idea.

bbpennel · 2023-12-05T15:31:35Z

Yes, thanks for working on this, unfortunately the new version requires ruby 3 while the yanked version required ruby 2.6, so this is causing dependency issues for us. I will most likely have to revert to 0.3.1

defnull · 2023-12-05T15:46:22Z

This should have been a breaking change. We have apps that use this gem on both sides of an API call to generate signatures and when one (and only one) side uses v 0.3.2 the signatures do not match.

You could also argue that 0.3.1 was a buggy version and fixing a bug (restoring expected behavior) is not a breaking change. SemVer also defines 0.x as unstable, expecting perfect backwards compatibility in a 0.3 release is a lot to ask.

sumirolabs · 2023-12-05T16:53:19Z

Agreed (I mentioned that above), this is a bit of a grey area. At the very least a CHANGELOG.md would have helped.

The maintainers yanked 0.3.2 version; so we're falling back to 0.3.1. Related to: - dryruby/json-canonicalization#2

renchap · 2023-12-05T16:57:49Z

The issue here is that the buggy version has been released for multiple months. Yanking this (buggy) version broke bundle install for every project using this version in their Gemfile.lock. This has a huge impact on many software projects, broke all the CIs, and for projects like us (Mastodon) who are releasing open-source software and required an emergency update.

Yanking a gem should only happen when there is a security issue with this specific version, and be done carefuly if your gem is used by other projects.

The maintainers yanked 0.3.2 version; so we're falling back to 0.3.1. Related to: - dryruby/json-canonicalization#2 - scientist-softserv/palni-palci#918

gkellogg · 2023-12-05T17:41:05Z

Obviously no good solution here. Can't restore 0.3.2, but could release a 0.3.3 which again uses the #to_json and runs on Ruby 2.7+.

I did what was asked, (see #2 (comment)) and 0.3.2 did arguably break things that it shouldn't have. Also note that this is a sole-maintained project with no one else jumping in to help (true for many open source projects). Anyone that's unhappy with how it's managed can feel free to copy the implementation entirely,, as it uses the UNLICENCE and is completely in the public domain. I would also welcome additional maintainers, as a process being reviewed by others would help for this and numerous other gems I maintain.

Options you can weigh in on:

release 0.3.3 as a duplicate of 0.3.1,
release 0.4.0 as a duplicate of 0.3.3 with support for Ruby 2.7
do nothing more and live with the changes.

gkellogg · 2023-12-05T17:42:41Z

Agreed (I mentioned that above), this is a bit of a grey area. At the very least a CHANGELOG.md would have helped.

As mentioned above, my practice is to use GitHub releases as the change log.

sumirolabs · 2023-12-05T19:35:45Z

I definitely appreciate the sole-proprietor nature here, no complaints. I'll let others weigh in on the path forward from here.

jscheid · 2023-12-05T20:45:24Z

Yanking this (buggy) version

There must be a misunderstanding - 0.3.2 wasn't buggy - it fixed a bug. I'm the one who opened this issue originally, and the PR in question, and I stand by it as a good improvement.

The change was yanked (for better or worse) not because there was anything wrong with it - only for considerations around how it should be versioned.

I wasn't involved in choosing which version to release it under, or in the decision to yank 0.3.2, but I'm sure @gkellogg only had best intentions and so did @sumirolabs, who presumably had already solved their own problem when they raised the versioning issue.

Let's use this incident to reflect on how we might want our package managers to work, and what guidance we should provide to package maintainers. For a start, should this perhaps not cheerfully say "here's how you can fix it" but rather "think twice before doing this"? There must be ways to limit the blast radius of versioning decisions and reduce pressure on individual maintainers.

As for which version to release next, I don't have a strong opinion.

…ds back support for Ruby >= 2.6. This version uses `#to_json` and retains the behavior that ActiveSupport may override the `#to_json` behavior in some cases. For #2

…y >= 2.6. This includes the use of `::JSON.generate` instead of `#to_json` and does not implicitly pick up ActiveSupport changes. For #2.

gkellogg · 2023-12-05T23:07:38Z

See PR #4 and #5. This creates a 0.3.3 release which is basically the same as 0.3.1 including support for Ruby >= 2.6. And 0.4.0, which is the old 0.3.2 and now 1.0.0, but also with support for Ruby >= 2.6. 1.0.0 and future work is on Ruby >= 3.0.

If people are "happy" with this, I'll push these out as gem versions. Sorry that RubyGems does not allow re-releasing a yanked gem version.

sumirolabs · 2023-12-06T14:05:12Z

This seems sensible to me.

gkellogg · 2023-12-08T02:31:06Z

Release 0.3.3 and 0.4.0 pushed to RubyGems as discussed.

@Performer

* ⚙️ Add auto-cleanup for GoodJob This commit introduces automatic clean-up of finished successful jobs. It is here to help with the overall performance of GoodJobs as well as the application database. Note the cleanup schedule is a guess on what's appropriate. <details><summary>Code Walk Through of Good Jobs Configuration</summary> The [README](https://github.com/bensheldon/good_job/blob/11b05e525d6cc0d4023b8b8b6b9824c40503b712/README.md?plain=1#L280) explains the configuration for the Scheduler. ```markdown - `cleanup_interval_seconds` (integer) Number of seconds a Scheduler will wait before cleaning up preserved jobs. Defaults to `nil`. Can also be set with the environment variable `GOOD_JOB_CLEANUP_INTERVAL_SECONDS`. ``` Here's the implementation of [GoodJob::Configuration](https://github.com/bensheldon/good_job/blob/11b05e525d6cc0d4023b8b8b6b9824c40503b712/lib/good_job/configuration.rb#L211-L220) regarding the cleanup_interval_seconds. Which is in the `GoodJob::CleanupTracker`. ```ruby def cleanup_interval_seconds value = ( rails_config[:cleanup_interval_seconds] || env['GOOD_JOB_CLEANUP_INTERVAL_SECONDS'] || DEFAULT_CLEANUP_INTERVAL_SECONDS ) value.present? ? value.to_i : nil end ``` The [GoodJob::CleanupTracker](https://github.com/bensheldon/good_job/blob/11b05e525d6cc0d4023b8b8b6b9824c40503b712/lib/good_job/cleanup_tracker.rb#L23-L29) has a `#cleanup?` method that looks at either job counts or elapsed seconds. Which informs the `GoodJob::Scheduler`. ```ruby def cleanup? (cleanup_interval_jobs && job_count > cleanup_interval_jobs) || (cleanup_interval_seconds && last_at < Time.current - cleanup_interval_seconds) || false end ``` The [GoodJob::Scheduler](https://github.com/bensheldon/good_job/blob/11b05e525d6cc0d4023b8b8b6b9824c40503b712/lib/good_job/scheduler.rb#L180-L193) observes the tasks as they complete. And one of those is conditionally running `#cleanup`. ```ruby def task_observer(time, output, thread_error) error = thread_error || (output.is_a?(GoodJob::ExecutionResult) ? output.unhandled_error : nil) GoodJob._on_thread_error(error) if error instrument("finished_job_task", { result: output, error: thread_error, time: time }) return unless output @cleanup_tracker.increment if @cleanup_tracker.cleanup? cleanup else create_task end end ``` The [GoodJob::Scheduler](https://github.com/bensheldon/good_job/blob/11b05e525d6cc0d4023b8b8b6b9824c40503b712/lib/good_job/scheduler.rb#L233-L250)'s `#cleanup` method delegates the clean_up to the performer; which is a `GoodJob::JobPerformer`. ```ruby def cleanup @cleanup_tracker.reset future = Concurrent::Future.new(args: [self, @Performer], executor: executor) do |_thr_scheduler, thr_performer| Rails.application.executor.wrap do thr_performer.cleanup end end observer = lambda do |_time, _output, thread_error| GoodJob._on_thread_error(thread_error) if thread_error create_task end future.add_observer(observer, :call) future.execute end ``` The [GoodJob::JobPerformer](https://github.com/bensheldon/good_job/blob/11b05e525d6cc0d4023b8b8b6b9824c40503b712/lib/good_job/job_performer.rb#L60-L64) then runs the general process `GoodJob.cleanup_preserved_jobs` (which is available via the CLI). ```ruby def cleanup GoodJob.cleanup_preserved_jobs end ``` The [GoodJob.cleanup_preserved_jobs](https://github.com/bensheldon/good_job/blob/11b05e525d6cc0d4023b8b8b6b9824c40503b712/lib/good_job.rb#L130-L153) method is the one that ultimately cleans up preserved jobs. Note that the `include_discarded` does some logical hoops with some grammatical antics (e.g. `old_jobs.not_discarded unless include_discarded`). We are not including discarded jobs so the query will limit to jobs that are not_discarded. ```ruby def self.cleanup_preserved_jobs(older_than: nil) configuration = GoodJob::Configuration.new({}) older_than ||= configuration.cleanup_preserved_jobs_before_seconds_ago timestamp = Time.current - older_than include_discarded = configuration.cleanup_discarded_jobs? ActiveSupport::Notifications.instrument("cleanup_preserved_jobs.good_job", { older_than: older_than, timestamp: timestamp }) do |payload| old_jobs = GoodJob::ActiveJobJob.where('finished_at <= ?', timestamp) old_jobs = old_jobs.not_discarded unless include_discarded old_jobs_count = old_jobs.count GoodJob::Execution.where(job: old_jobs).destroy_all payload[:destroyed_records_count] = old_jobs_count end end ``` </details> Related to: - scientist-softserv/adventist-dl#690 * 🧹 Pin to json-canonicalization 0.3.1 The maintainers yanked 0.3.2 version; so we're falling back to 0.3.1. Related to: - dryruby/json-canonicalization#2 - scientist-softserv/palni-palci#918 * Bulkrax upgrade to remove duplicates when re-importing (#554) * upgrade bulkrax to be less likely to miss existing works when searching for them * ⚙️ Upgrade to v6.0.0 of Bulkrax --------- Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com> * add goodjob clean up to deploy --------- Co-authored-by: Rob Kaufman <rob@notch8.com>

dryruby/json-canonicalization#2

This is actually not changing anything. The author didn't realise the bad implications of yanking 0.3.2 and restored it as 0.4.0. dryruby/json-canonicalization#2

The maintainers yanked 0.3.2 version (see dryruby/json-canonicalization#2)

In prod, Helvetica Neue is the default font, but in knapsack it was Nobile. The client has requested feature parity with prod. Issue: - https://github.com/scientist-softserv/adventist-dl/issues/664 Related (Prod): - https://github.com/scientist-softserv/adventist-dl/blob/7c2c64939044d61fd3a210e95fc3bcba761b79ae/app/forms/hyrax/forms/admin/appearance.rb#L14-L17 update hyrax-webapp json-canonicalization gem The maintainers yanked 0.3.2 version (see dryruby/json-canonicalization#2)

This is actually not changing anything. The author didn't realise the bad implications of yanking 0.3.2 and restored it as 0.4.0. dryruby/json-canonicalization#2

jscheid added the bug Something isn't working label May 2, 2023

jscheid added a commit to jscheid/json-canonicalization that referenced this issue May 2, 2023

Compatibility with ActiveSupport

93e9285

Closes dryruby#2

jscheid mentioned this issue May 2, 2023

Compatibility with ActiveSupport #3

Merged

gkellogg closed this as completed in #3 May 2, 2023

gkellogg pushed a commit that referenced this issue May 2, 2023

Compatibility with ActiveSupport

f024f3a

Closes #2

gkellogg reopened this Dec 4, 2023

gkellogg closed this as completed Dec 4, 2023

theothertom mentioned this issue Dec 4, 2023

Docker build fails: json-canonicalization 0.3.2 is missing mastodon/mastodon#28224

Closed

ashwinisukale mentioned this issue Dec 5, 2023

json-canonicalization 0.3.2 is removed from rubygems repo datacite/bolognese#175

Closed

jeremyf added a commit to scientist-softserv/palni-palci that referenced this issue Dec 5, 2023

🧹 Pin to json-canonicalization 0.3.1

eb1779b

The maintainers yanked 0.3.2 version; so we're falling back to 0.3.1. Related to: - dryruby/json-canonicalization#2

jeremyf mentioned this issue Dec 5, 2023

🧹 Pin to json-canonicalization 0.3.1 scientist-softserv/palni-palci#918

Merged

jeremyf added a commit to scientist-softserv/utk-hyku that referenced this issue Dec 5, 2023

🧹 Pin to json-canonicalization 0.3.1

7a3367b

The maintainers yanked 0.3.2 version; so we're falling back to 0.3.1. Related to: - dryruby/json-canonicalization#2 - scientist-softserv/palni-palci#918

jeremyf mentioned this issue Dec 5, 2023

🧹 Pin to json-canonicalization 0.3.1 scientist-softserv/utk-hyku#575

Merged

gkellogg mentioned this issue Dec 5, 2023

Updates for release 0.3.3 #4

Closed

gkellogg added a commit that referenced this issue Dec 5, 2023

Updates for version 0.4.0, which is 1.0.0 but retains support for Rub…

19edc4a

…y >= 2.6. This includes the use of `::JSON.generate` instead of `#to_json` and does not implicitly pick up ActiveSupport changes. For #2.

gkellogg mentioned this issue Dec 5, 2023

Updates for version 0.4.0 #5

Closed

aprilrieger mentioned this issue Dec 6, 2023

pin-json-canonicalization-0.3.1 samvera/hyku#2044

Merged

tommy-gilligan added a commit to tommy-gilligan/openfoodnetwork that referenced this issue Dec 10, 2023

move off yanked version off json-canonicalization

39ae764

dryruby/json-canonicalization#2

mkllnk mentioned this issue Dec 10, 2023

[BUU] Sticky table header and saving banner openfoodfoundation/openfoodnetwork#11902

Closed

mkllnk mentioned this issue Dec 10, 2023

Bump json-canonicalization from 0.3.2 to 0.4.0 openfoodfoundation/openfoodnetwork#11917

Merged

4 tasks

conorom mentioned this issue Dec 14, 2023

solve bundle failure due to yanked gem mlibrary/heliotrope#3551

Merged

ShanaLMoore added a commit to scientist-softserv/adventist_knapsack that referenced this issue Feb 12, 2024

update hyrax-webapp json-canonicalization gem

3aac844

The maintainers yanked 0.3.2 version (see dryruby/json-canonicalization#2)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bad results with Rails #2

Bad results with Rails #2

jscheid commented May 2, 2023

jscheid commented May 2, 2023

gkellogg commented May 2, 2023

jscheid commented May 2, 2023

sumirolabs commented Dec 1, 2023

gkellogg commented Dec 1, 2023

sumirolabs commented Dec 4, 2023 •

edited

Loading

gkellogg commented Dec 4, 2023

sumirolabs commented Dec 4, 2023

gkellogg commented Dec 4, 2023

gkellogg commented Dec 4, 2023

renchap commented Dec 5, 2023 •

edited

Loading

sumirolabs commented Dec 5, 2023

bbpennel commented Dec 5, 2023

defnull commented Dec 5, 2023 •

edited

Loading

sumirolabs commented Dec 5, 2023

renchap commented Dec 5, 2023

gkellogg commented Dec 5, 2023

gkellogg commented Dec 5, 2023

sumirolabs commented Dec 5, 2023

jscheid commented Dec 5, 2023

gkellogg commented Dec 5, 2023

sumirolabs commented Dec 6, 2023

gkellogg commented Dec 8, 2023

Bad results with Rails #2

Bad results with Rails #2

Comments

jscheid commented May 2, 2023

jscheid commented May 2, 2023

gkellogg commented May 2, 2023

jscheid commented May 2, 2023

sumirolabs commented Dec 1, 2023

gkellogg commented Dec 1, 2023

sumirolabs commented Dec 4, 2023 • edited Loading

gkellogg commented Dec 4, 2023

sumirolabs commented Dec 4, 2023

gkellogg commented Dec 4, 2023

gkellogg commented Dec 4, 2023

renchap commented Dec 5, 2023 • edited Loading

sumirolabs commented Dec 5, 2023

bbpennel commented Dec 5, 2023

defnull commented Dec 5, 2023 • edited Loading

sumirolabs commented Dec 5, 2023

renchap commented Dec 5, 2023

gkellogg commented Dec 5, 2023

gkellogg commented Dec 5, 2023

sumirolabs commented Dec 5, 2023

jscheid commented Dec 5, 2023

gkellogg commented Dec 5, 2023

sumirolabs commented Dec 6, 2023

gkellogg commented Dec 8, 2023

sumirolabs commented Dec 4, 2023 •

edited

Loading

renchap commented Dec 5, 2023 •

edited

Loading

defnull commented Dec 5, 2023 •

edited

Loading