Change JSONFields to more specific Fields

[pedro-psb]

Closes: #3657

---

Previous Problem this was trying to solve

Problem

As mentioned in #3639, the DRF JSONFields are too generic and can be any JSON primitive (str, bool, etc), not just dicts or lists (as our codebase appears to be interpreting it).

After drf-spectacular fixed that interpretation on their side, our openapi schema broke because Object from JSONField became Any, which matches what JSON Fields are in DRF but not what we think it meant before.

Approaches considered

1. Change all JSONField to some other fields that represents better what the field actually is. Example, repo_config is a DictField, some listy object should be ListField, etc.
2. Use extend_schema to modify how the openapi schema will be generated. It affect only schema generation, AFAIK.
3. Force JSONField to be a OA 'object', as suggested here. The downside is that the representation of our fields would continue to be inaccurate.

[dralley]

What do you mean by "The downside is that the representation of our fields would continue to be inaccurate."?

[pedro-psb]

For example, the Repository.repo_config is a JsonFIeld. We want it be a json "Object Type" (or dict), but the JSONField serializer will happily accept a list or any json primitives, such as ints, booleans.

Edit: but given all those test failures, I'm thinking about doing the alternative fix and create another issue about reviewing the JSONFields, since using them has worked so far. Wdyt?

[mdellweg]

IMHO Option 3. is a non starter, because it leaks into the whole codebase and affects other plugins too. Have you considered updating the bindings container? Maybe there is a newer version that knows how to deal with Any. But anyway, if you know that repo-config is supposed to be a dictionary, than that is exactly the type of change we should do to the schema here.

[pedro-psb]

IMHO Option 3. is a non starter, because it leaks into the whole codebase and affects other plugins too.

Hmm, I didnt know about that.

Have you considered updating the bindings container? Maybe there is a newer version that knows how to deal with Any.

Will check that out.

But anyway, if you know that repo-config is supposed to be a dictionary, than that is exactly the type of change we should do to the schema here.

Yes, it is. I started by naively changing all the fields, but that broke the tests. It'll require a more thoughtful approach.

[dralley]

For the sake of documenting it on Github, re: the discussion we had on Slack, is it correct to say that upgrading openapi-generator-cli to >=5.30 will resolve this issue?

(but with some risk of breakage it sounds like - in terms of backports)

[mdellweg]

For the sake of documenting it on Github, re: the discussion we had on Slack, is it correct to say that upgrading openapi-generator-cli to >=5.30 will resolve this issue?

(but with some risk of breakage it sounds like - in terms of backports)

From the looks of it, yes. The risk with updating the openapi-generator templates is, that it immediately affects all new and old versions of all of Pulp plugins. I'm unsure if we are ready to do that.

[pedro-psb]

I'm working on getting it pass our basic tests here: https://github.com/pulp/pulp-openapi-generator/pull/101

About the break risk, we could ask katello to run some tests with the updated bindings before we do anything.

[dralley]

The risk with updating the openapi-generator templates is, that it immediately affects all new and old versions of all of Pulp plugins. I'm unsure if we are ready to do that.

Well that would seem to be a long-term problem, surely we need to update the templates eventually.

[mdellweg]

The risk with updating the openapi-generator templates is, that it immediately affects all new and old versions of all of Pulp plugins. I'm unsure if we are ready to do that.

Well that would seem to be a long-term problem, surely we need to update the templates eventually.

Absolutely. And actually I think we may need to take different versions of it for different versions of pulpcore. My take is, improving the annotations of the individual fields here and now is a fast solution that is also future proof and worth doing in any scenario.

[pedro-psb]

Given those considerations, I'll back to work on this as an immediate solution to the broken bindings.

I made progress on the work with fixing the bump regressions, but I reached a deadline there where I don't know where to go (without getting really deep into openapi stuff).

[stale[bot]]

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

[stale[bot]]

This pull request has been closed due to inactivity. If you feel this is in error, please reopen the pull request or file a new PR with the relevant details.