Upgrade dependencies (reform, roar, representable and others)

[mayorova]

What this PR does / why we need it:

Upgrade various dependencies. This was extracted from #3705 to reduce the scope.

This was initially started as an upgrade for the reform gem, but it pulled updated versions (including major versions update) of its dependencies... including:

• representable
• roar
• roar-rails
• responders

and some others.

The reasoning for the roar upgrade is that ActiveRecord 6.1 support was added in reform-rails v0.2.2: https://github.com/trailblazer/reform-rails/blob/master/CHANGES.md#022

And the split between reform and reform-rails happened even earlier.

I think the old version did actually work with Active Record 6.1 (at least the tests were passing), but I noticed lots of deprecation warnings (mostly related to ActiveModel::Errors), so I thought an upgrade would be nice, especially as we moving towards upgrading Ruby to v3, it's better keep the dependencies up-to-date.
This upgrade though required a non-trivial fix in the ThreeScale::Reform module (see 327ed11), which I am not sure whether it's a good fix.

Which issue(s) this PR fixes

-none-

Verification steps

Special notes for your reviewer:

[jlledom]

Is this a part of #3705?

[mayorova]

Is this a part of #3705?

Yes @jlledom If we can, we upgrade these first, and then just bump rails.

[mayorova]

I found this change pretty tricky, because I don't really understand the documentation of the reform gem very well (not sure if it's mine, or the documentation's fault).

This seems to do the trick, although:

1. I am not sure if the behavior is 100% equivalent
2. The code says The #errors method will be removed in Reform 3.0 core. def errors(*args) https://github.com/trailblazer/reform/blob/v2.6.2/lib/reform/contract/validate.rb#L14

Also, there are just three forms, it seems, that use reform,and honestly, I think it might be possible to get rid of it at some point and simplify the implementation.

I think for the current purposes this rework should be enough, but probably we can refactor/remove this later.

[jlledom]

Which was the problem here? How can I verify it works the same way?

[mayorova]

The problem was that after the upgrade self.class.representer_class.representable_attrs[:definitions] did not exist anymore. The internal data structures in the library changed a lot, and the field definitions are now stored in the schema.

I'll try to come of with a short snippet that shows the behavior.

[mayorova]

@jlledom So, these are the forms that use Reform:

• Notification Preferences - here I don't even know how it uses it, because it doesn't use the Reform's API (property :name etc.).
• the other two belong to Onboarding wizard

https://github.com/search?q=repo%3A3scale%2Fporta+include+ThreeScale%3A%3AReform&type=code

From what I can see the errors are not even visualized in the UI, so this merge_errors method seems to be pretty useless to me.

However, I added a couple of tests here to prove that the method works (without merge_errors the test fail), and I also confirmed via manual testing that the behavior is the same as in master. Probably except I am converting the field name to symbol for consistency with ActiveModel::Errors, but again, we're not relying on this anywhere, from what I can tell.

[mayorova]

This change is quite confusing...

But this is basically a copy from https://github.com/trailblazer/representable/blob/v3.0.4/lib/representable.rb#L63-L65

I added it because otherwise, the execution ended up using this method here: https://github.com/trailblazer/representable/blob/v3.0.4/lib/representable/cached.rb#L18-L20

And it was just using a wrong binding... Unfortunately, I don't remember the exact error anymore 😅 but I'm running a test suite without this change to see whether it's still needed after other fixes, see https://app.circleci.com/pipelines/github/3scale/porta/27844/workflows/dbd93801-7760-49a4-8d37-46d32ca11aeb

UPDATE: here we go, it was document must be a Nokogiri::XML::Node. I think this was because for some reason XML binding was used even if the format was json. I don't know if that's the proper fix, but it works. This probably deserves a comment in the code, but I am not sure how to make it meaningful 😅

[jlledom]

How could I reproduce the error if I remove this change?

[mayorova]

You can see the tests that fail in https://app.circleci.com/pipelines/github/3scale/porta/27844/workflows/dbd93801-7760-49a4-8d37-46d32ca11aeb

[jlledom]

I'm surprised with this, it looks like a bug in the gem or something. I mean, I assume here they intend to call this representable_map, however they include representable/cached which apparently overrides representable_map. Then, why they define the original representable_map in the first place? How could it ever get called? And how is it possible nobody noticed this before?

[mayorova]

Yeah, I don't understand it either...

This commit message is interesting trailblazer/representable@b26aec4

ALWAYS use Cached in Decorator. this might break things when people later include the format engine module, or include multiple of the latter. please contact me then.

"when people later include the format engine module" - include where? what is the "format engine module"?... I don't get it, but maybe we need to contact the guy? 😆

[mayorova]

OK, I think I understood what this is about, and I think I've applied a proper fix now: 00e1695

[jlledom]

The changes look good but there are still tests failing.

[mayorova]

Yeah... apparently this broke something, and now the provider is rendered as:

<authentication_providers>
    <authentication_provider id="3" kind="keycloak" account_type="provider" name="Red Hat Single Sign-On" system_name="keycloak_d956bd3b2398" client_id="something" client_secret="hello" site="https://keycloak-daria-tools.apps.dev-eng-ocp4.dev.3sca.net/auth/realms/threescale" authorize_url="https://keycloak-daria-tools.apps.dev-eng-ocp4.dev.3sca.net/auth/realms/threescale/protocol/openid-connect/auth?client_id=something&amp;redirect_uri=http%3A%2F%2Fprovider-admin.3scale.localhost%3A3000%2Fauth%2Fkeycloak_d956bd3b2398%2Fcallback%3Faccess_token%3Dadmintoken&amp;response_type=code&amp;scope=openid" skip_ssl_certificate_verification="false" automatically_approve_accounts="false" account_id="2" username_key="login" identifier_key="id" trust_email="false" published="true" created_at="2023-07-12T12:07:41Z" updated_at="2023-07-26T09:21:24Z" callback_url="http://provider-admin.3scale.localhost:3000/auth/keycloak_d956bd3b2398/callback"/>
    <authentication_provider id="4" kind="auth0" account_type="provider" name="Auth0" system_name="auth0_1c1533fbac6a" client_id="a" client_secret="b" site="https://whatever.auth0.com" authorize_url="https://mayorova.auth0.com/authorize?client_id=a&amp;redirect_uri=http%3A%2F%2Fprovider-admin.3scale.localhost%3A3000%2Fauth%2Fauth0_1c1533fbac6a%2Fcallback%3Faccess_token%3Dadmintoken&amp;response_type=code&amp;scope=openid+profile+email" skip_ssl_certificate_verification="false" automatically_approve_accounts="false" account_id="2" username_key="login" identifier_key="id" trust_email="false" published="false" created_at="2023-08-08T12:32:15Z" updated_at="2023-08-08T12:32:15Z" callback_url="http://provider-admin.3scale.localhost:3000/auth/auth0_1c1533fbac6a/callback, http://provider-admin.3scale.localhost:3000/auth/invitations/auth0/auth0_1c1533fbac6a/callback"/>
</authentication_providers>

[mayorova]

@jlledom I looked at it again, and the "proper fix" is not proper actually 😅

So, the initial problem was that the XML and JSON bindings were used incorrectly because of caching, and serializing a JSON was causing document must be a Nokogiri::XML::Node.

After the "fix" the caching was still in place, and so the generic Hash binding was used, because it was cached on the representer initialization. Which does not crash, but does cause a wrongly formatted XML.

I actually found this issue, which is example the same thing we're dealing with: trailblazer/representable#180

And the response of the maintainer is: trailblazer/representable#180 (comment)

So, I returned to the original solution, and added more comments to explain what this change does.

[jlledom]

OK, I understand the problem. It would be good for representable to provide a way to disable that cache, but if there's no way, we need this.

[mayorova]

Not sure whether there is any issue with just calling this. I guess the problem is that it changes the original representable_attrs, but I don't know whether that can be problematic.

[jlledom]

After spending a couple of hours with this, I don't think it's worth trying to understand every change in the PR, they are all ad-hoc fixes to breaking changes in the gems, probably because we are messing with the gems internals. If tests pass, I think it should be fine.