Workaround psych 4 issue

[cjcolvar]

Test with newer rails versions and pin psych in rails 5.1 and 5.2

[jrochkind]

This stuff is very confusing; I'm wondering what has changed since the work @eddierubeiz did to get green CI on then-latest ruby and Rails (through 6.1) versions at #371 (and prior building blocks), to require this additional work. Is main branch actually currently failing, event though it was passing at #371? Or just failing when upgrading to latest patch releases of ruby/rails? Either are confusing to me. Or other? Anyone have a sense of what's actually going on here?

[jrochkind]

As I'm trying to figure out what's going on....

We had previously added a psych restriction but in Gemfile.extra rather than Gemfile -- Gemfile.extra is a file used by engine_cart, and we thought the proper place to put this.

https://github.com/samvera/questioning_authority/blob/568382478a31cbb901aa58c7087d25e9023017bf/spec/test_app_templates/Gemfile.extra

Added in this commit: https://github.com/samvera/questioning_authority/commit/65974f8e70a9aa3dd5cf2e89c85886f7de2559d9

Note that the Gemfile.extra only restricts psych for Rails 6.1 (and ruby 3.1), which was all that seemed neccesary to us -- here you are restricting it for Rails 5 too? It's somehow necessary even though Rails 5 isn't being tested with ruby 3.1 (since it doesn't work with ruby 3.1)?

But okay, if we assume for now that wider restriction on psych when testing is needed -- we should probably avoid splitting psych restrictions across Gemfile and Gemfile.extra, with some in one and some in the other. I'm honestly not sure which is the "right" place, but either way we should have them both in the same place, to avoid making things even more confusing for future devs including future us?

Still very confused how main is failing now when we thought we left it passing a month ago. (were we wrong?)

[cjcolvar]

I'm mystified as well. I didn't realize that the psych restriction was in Gemfile.extra and so this is my grasping at anything that would fix the build in main. I don't have anything that I'm hoping to achieve here other than fix the build which has been failing nightly since August 4th.

Last successful build: https://app.circleci.com/pipelines/github/samvera/questioning_authority/571 First failing build: https://app.circleci.com/pipelines/github/samvera/questioning_authority/572 First failing build with psych error: https://app.circleci.com/pipelines/github/samvera/questioning_authority/577

I'm not sure what's different about those. From what I can tell it looks like up to build 576 psych wasn't getting pulled in by bundler and with build 577 and after version 4.0.4 is getting pulled in.

[jrochkind]

Thanks for context update @cjcolvar.

Yeah, it looks to us like with no code changes in this repo, the build went from passing to failing from builds 571 to 572-- I guess must be because of change(s) in dependency(ies) we have yet to identify?

Very mysterious and annoying.

Maybe we can find some more time to work on it too, not sure.

Also not entirely sure if I understand what psych 4 is incompatible with -- I know it's incompatible with certain "old Rails" (which won't change, those Rails do just need to lock psych to 3), but I'm not sure what versions, or if that is the entirety of the problems.

[jrochkind]

Hm, that it first started failing without an error clearly mentioning psych, but only later did... seems also troubling.

This is making me think of the YAML-involved CVE that resulted in Rails seurity releases. https://securityonline.info/cve-2022-32224-ruby-on-rails-remote-code-execution-vulnerability/

Could it have started failing just when the patch releases were released? Ie, with no code changes here, passing on (eg) Rails 6.1.6, but failing on 6.1.6.1 after it's released and it's using that? Hmm, that does not seem to the case though, first failing build seems to still be using Rails 6.1.1 in a failed run.

Odd odd odd. This is a challenging codebase to maintain, as I think there are lots of somewhat complicated parts of the code (often involving RDF), that I think few if any remaining community members understand. In this case, I don't really even understand the RDF-related code under test of the test that's failing.

[jrochkind]

OK I'm working on this a bit.

There are definitely two problems -- the psych 4 problem, PLUS once that is solved there is still another one (the error from the FIRST failing CI).

Very weird. Trying to look into it. Challenging becuase I don't understand the code that is failing the test -- don't really understand what it's supposed to do, or how it's implemented. Some linked data madness.

[jrochkind]

OK this is weird. The test is testing a raw (serialized to string) JSON string, to see if it contains the sub-string "Rodgers & Hart"

What the raw serialized JSON string actually includes, at least in matrix elements that are failing tests, is "Rodgers \u0026 Hart". \u0026 when parsed by a JSON parser, turns into a &.

But testing the actual raw string, it does not include "Rodgers & Hart".

So it's kind of a mystery a) Why this passes SOME ruby/rails version combos; b) why this ever passed....

Maybe in some dependency combos, the raw string does NOT have "\u0026" in it -- not sure where this raw string is actually coming from, or why it would end up escaping &, which does not have to be escaped in JSON. Confirmed that in ruby 2.5/rails 5.2, literal "&" is in raw source string instead of "\u0026" -- don't know why this changed, how this used to pass even in later rubies.

But I think I'm going to treat this as a test failure, not a code failure -- since the string still parses to "Rodgers & Hart" -- I'm just going to change the test to be on the parsed values, not the raw values.

I also don't understand why the code-under-test (authority.find("7143179", tc)) is returning serialized JSON strings that need to be parsed in the first place, but that is what the test is testing for. I do not know if this code actually works to do anything useful or is actually in use or not.

Any thoughts @cjcolvar?

[jrochkind]

Replaced by #374