Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repo creation errors out with Ruby bindings #3639

Closed
ianballou opened this issue Jun 24, 2024 · 11 comments
Closed

Repo creation errors out with Ruby bindings #3639

ianballou opened this issue Jun 24, 2024 · 11 comments
Assignees
Labels

Comments

@ianballou
Copy link
Contributor

Version

pulp-rpm 3.27.1 (client has same version)

      "versions": {
        "deb": "3.2.0",
        "rpm": "3.27.1",
        "core": "3.49.9",
        "file": "3.49.9",
        "ostree": "2.3.0",
        "python": "3.11.1",
        "ansible": "0.21.5",
        "certguard": "3.49.9",
        "container": "2.20.0"
      }

Tested with Katello 4.14.develop

Describe the bug
Creating a repository returns a traceback. The repo does get created, but there's an error.

[11] pry(main)> repoconfig = PulpRpmClient::RpmRpmRepository.new(name: 'tes2')
=> #<PulpRpmClient::RpmRpmRepository:0x00007f497e8b0fa0 @autopublish=false, @name="tes2", @package_signing_fingerprint="">
[12] pry(main)> ::Katello::Repository.yum_type.first.backend_service(SmartProxy.pulp_primary).api.repositories_api.create(repoconfig)
2024-06-24T19:54:03 [D|kat|] Calling API: RepositoriesRpmApi.create ...
2024-06-24T19:54:03 [D|kat|] HTTP request body param ~BEGIN~
 | {"name":"tes2","autopublish":false,"package_signing_fingerprint":""}
 | ~END~
 | 
2024-06-24T19:54:03 [D|kat|] HTTP response body ~BEGIN~
 | {"pulp_href":"/pulp/api/v3/repositories/rpm/rpm/01904bd0-096a-7a74-8630-a4ed94672c82/","pulp_created":"2024-06-24T19:54:03.499852Z","pulp_last_updated":"2024-06-24T19:54:03.513474Z","versions_href":"/pulp/api/v3/repositories/rpm/rpm/01904bd0-096a-7a74-8630-a4ed94672c82/versions/","pulp_labels":{},"latest_version_href":"/pulp/api/v3/repositories/rpm/rpm/01904bd0-096a-7a74-8630-a4ed94672c82/versions/0/","name":"tes2","description":null,"retain_repo_versions":null,"remote":null,"autopublish":false,"metadata_signing_service":null,"package_signing_service":null,"package_signing_fingerprint":"","retain_package_versions":0,"checksum_type":null,"metadata_checksum_type":null,"package_checksum_type":null,"gpgcheck":null,"repo_gpgcheck":null,"sqlite_metadata":false,"repo_config":{},"compression_type":null}
 | ~END~
 | 
NameError: uninitialized constant PulpRpmClient::AnyType
from /home/vagrant/foreman/.vendor/ruby/3.0.0/gems/pulp_rpm_client-3.27.1/lib/pulp_rpm_client/models/rpm_rpm_repository_response.rb:491:in `const_get'

To Reproduce
Try to create an RPM repo with the Ruby bindings

Expected behavior
Repo is created without an error.

@mdellweg
Copy link
Member

I suspect the pulpcore version at the time of creating the bindings may have been different from 3.49.9.

See also https://discourse.pulpproject.org/t/we-will-stop-publishing-bindings-soon/1240

@ianballou
Copy link
Contributor Author

If that's the cause, even more reason for us to start generating the bindings ourselves. I can try pulp-rpm 3.26 for now if that would be preferable.

@ianballou
Copy link
Contributor Author

I've confirmed that regenerating the bindings myself fixes the issue. In comparing the generated bindings with the bindings on rubygems.org, I'm seeing references to AnyType that seem to be the cause of the problem:

diff -r /home/vagrant/foreman/.vendor/ruby/3.0.0/gems/pulp_rpm_client-3.27.1/lib/pulp_rpm_client/models/rpm_package_response.rb ../pulp-openapi-generator/pulp_rpm-client/lib/pulp_rpm_client/models/rpm_package_response.rb
230,239c230,239
<         :'changelogs' => :'AnyType',
<         :'files' => :'AnyType',
<         :'requires' => :'AnyType',
<         :'provides' => :'AnyType',
<         :'conflicts' => :'AnyType',
<         :'obsoletes' => :'AnyType',
<         :'suggests' => :'AnyType',
<         :'enhances' => :'AnyType',
<         :'recommends' => :'AnyType',
<         :'supplements' => :'AnyType',
---
>         :'changelogs' => :'Object',
>         :'files' => :'Object',
>         :'requires' => :'Object',
>         :'provides' => :'Object',
>         :'conflicts' => :'Object',
>         :'obsoletes' => :'Object',
>         :'suggests' => :'Object',
>         :'enhances' => :'Object',
>         :'recommends' => :'Object',
>         :'supplements' => :'Object',

and

diff -r /home/vagrant/foreman/.vendor/ruby/3.0.0/gems/pulp_rpm_client-3.27.1/lib/pulp_rpm_client/models/rpm_package_environment_response.rb ../pulp-openapi-generator/pulp_rpm-client/lib/pulp_rpm_client/models/rpm_package_environment_response.rb
81,84c81,84
<         :'group_ids' => :'AnyType',
<         :'option_ids' => :'AnyType',
<         :'desc_by_lang' => :'AnyType',
<         :'name_by_lang' => :'AnyType',
---
>         :'group_ids' => :'Object',
>         :'option_ids' => :'Object',
>         :'desc_by_lang' => :'Object',
>         :'name_by_lang' => :'Object',

for examples.

@ianballou
Copy link
Contributor Author

We (Katello) still need to come up with a solid plan to create our own Ruby bindings. Building the bindings ourselves is easy as a one-off, but we'd need our self-generation process ironed out before we can start doing it reliably. Is there any way this can be handled without us having to monkey-patch things?

@dralley
Copy link
Contributor

dralley commented Jun 27, 2024

Is there any way this can be handled without us having to monkey-patch things?

@ianballou Are you talking about the short-term (handling this specific issue) or the long-term (working on the self-generation process)?

We'd be happy to work with you on getting the long-term process sorted out.

@ianballou
Copy link
Contributor Author

@dralley I was talking about the short term. For our upcoming Katello 4.14, if we need to generate our own bindings, it would need to be done manually each time. It's not impossible, but there's always the risk of our packaging automation being used to bump the pulp_rpm_client version and having a new version pulled from rubygems.

@pedro-psb pedro-psb reopened this Jun 28, 2024
@pedro-psb
Copy link
Member

The problem

After some investigation and experimentation, these are my conclusions:

  • the upgrade to drf-spectacular 0.27.2 on the Y pulpcore release (3.55.0) caused this regression
  • the diff between the api.json files is that the regression is missing a type attribute on some fields:
diff --git a/tmp/wrong-api.json b/tmp/right-api.json
index fc0ae2d..5fd2c53 100644
--- a/tmp/wrong-api.json
+++ b/tmp/right-api.json
@@ -9066,6 +9066,7 @@
       "description": "A serializer for Content Copy API.",
       "properties": {
         "config": {
+          "type": "object",
           "description": "A JSON document describing sources, destinations, and content to be copied"
         },
         "dependency_solving": {
@@ -10104,6 +10105,7 @@
           "minimum": 0
         },
         "repo_config": {
+          "type": "object",
           "description": "A JSON document describing config.repo file"
         },
# ...
# The rest is very similar

The experiment was:

  • case: AnyTypes on the gem
    • with pulpcore from main
    • with rpm on main
  • case: no AnyTypes on the gem
    • with pulpcore from main but with the drf-spectacular bump commit dropped via rebase
    • with rpm on main

Ideas

I'm not sure what is the way to go, but some general approaches I see are:

  1. try to get to the root cause of the problem
    • couldn't find in the drf-spectacular changes any obvious culprit
  2. do some hack on the workflow:
    • patch the api.json or gem
    • patch pulpcore, possibly only the binding publication part
  3. Revert the bump

@mdellweg
Copy link
Member

Actually not specifying the type: object is correct from the openapiv3 specification side. This is a bug that was fixed in spectacular. A JSONField can hold any type, including lists and strings, numbers and booleans. "object" from the JSON Schema Specification however only allows dictionaries.
My conclusion is, that the ruby templates in openapi-generator (or at least the ancient version we currently employ) do not handle that valid case properly.
But I would still suggest to go into the viewset and add @extend_schema decorators, defining what exactly the plugin expects to see in that json field.

tfranzel/drf-spectacular#1095

@dralley
Copy link
Contributor

dralley commented Jun 30, 2024

@pedro-psb Wow. Great work!

pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue Jul 1, 2024
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue Jul 1, 2024
Drf JSONField is actually 'any' because it can be of any type (list, obj
int, bool, etc). Nevertheless, we (and drf-spectacular) always interpret
it as being of type 'object'.

In v0.27.0, drf-spectacular fixed that on their side, which broke us.

Although it is not correct to keep using JSONFields strictly as a object,
this commit adds a drf-spectacular extension that enforces that it should
always be trated as an 'object' on openapi schema generation.

The reason is to provide a safe fix for our users, who need their
bindings working the same way as they always did.

Closes: pulp#3639
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue Jul 1, 2024
Drf JSONField is actually 'any' because it can be of any type (list, obj
int, bool, etc). Nevertheless, we (and drf-spectacular) always interpret
it as being of type 'object'.

In v0.27.0, drf-spectacular fixed that on their side, which broke us.

Although it is not correct to keep using JSONFields strictly as a object,
this commit adds a drf-spectacular extension that enforces that it should
always be trated as an 'object' on openapi schema generation.

The reason is to provide a safe fix for our users, who need their
bindings working the same way as they always did.

Closes: pulp#3639
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue Jul 1, 2024
Drf JSONField is actually 'any' because it can be of any type (list, obj
int, bool, etc). Nevertheless, we (and drf-spectacular) always interpret
it as being of type 'object'.

In v0.27.0, drf-spectacular fixed that on their side, which broke us.

Although it is not correct to keep using JSONFields strictly as a object,
this commit adds a drf-spectacular extension that enforces that it should
always be trated as an 'object' on openapi schema generation.

The reason is to provide a safe fix for our users, who need their
bindings working the same way as they always did.

Closes: pulp#3639
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue Jul 1, 2024
Drf JSONField is actually 'any' because it can be of any type (list, obj
int, bool, etc). Nevertheless, we (and drf-spectacular) always interpret
it as being of type 'object'.

In v0.27.0, drf-spectacular fixed that on their side, which broke us.

Although it is not correct to keep using JSONFields strictly as a object,
this commit adds a drf-spectacular extension that enforces that it should
always be trated as an 'object' on openapi schema generation.

The reason is to provide a safe fix for our users, who need their
bindings working the same way as they always did.

Closes: pulp#3639
@pedro-psb pedro-psb self-assigned this Jul 1, 2024
@pulpbot pulpbot moved this from Done to In Progress in RH Pulp Kanban board Jul 1, 2024
@pedro-psb
Copy link
Member

I've confirmed that using pulp-openapi-generator >= 5.3.0 fixes that (OpenAPITools/openapi-generator#10192).

pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue Jul 9, 2024
The JSONField was yielding an empty type on the schema, which
broke bindings generation.

This replaces drf serializers.JSONField with a custom one that is an
OpenApi 'object' type.

Closes: pulp#3639
patchback bot pushed a commit to pulp/pulpcore that referenced this issue Oct 29, 2024
patchback bot pushed a commit to pulp/pulpcore that referenced this issue Oct 29, 2024
mdellweg pushed a commit to pulp/pulp_ansible that referenced this issue Oct 29, 2024
mdellweg pushed a commit to pulp/pulp_ansible that referenced this issue Oct 29, 2024
mdellweg pushed a commit to pulp/pulp_ansible that referenced this issue Oct 29, 2024
pedro-psb added a commit to pedro-psb/pulpcore that referenced this issue Oct 29, 2024
pedro-psb added a commit to pedro-psb/pulpcore that referenced this issue Oct 29, 2024
pedro-psb added a commit to pedro-psb/pulpcore that referenced this issue Oct 29, 2024
mdellweg pushed a commit to pulp/pulp_ansible that referenced this issue Oct 29, 2024
mdellweg pushed a commit to pulp/pulpcore that referenced this issue Oct 30, 2024
mdellweg pushed a commit to pulp/pulpcore that referenced this issue Oct 30, 2024
mdellweg pushed a commit to pulp/pulpcore that referenced this issue Oct 30, 2024
mdellweg pushed a commit to pulp/pulpcore that referenced this issue Oct 30, 2024
mdellweg pushed a commit to pulp/pulpcore that referenced this issue Oct 30, 2024
mdellweg pushed a commit to pulp/pulpcore that referenced this issue Oct 30, 2024
pedro-psb added a commit to pedro-psb/pulp_ansible that referenced this issue Oct 31, 2024
pedro-psb added a commit to pedro-psb/pulp_ansible that referenced this issue Oct 31, 2024
pedro-psb added a commit to pedro-psb/pulp_ansible that referenced this issue Oct 31, 2024
mdellweg pushed a commit to pulp/pulp_ansible that referenced this issue Oct 31, 2024
mdellweg pushed a commit to pulp/pulp_ansible that referenced this issue Oct 31, 2024
mdellweg pushed a commit to pulp/pulp_ansible that referenced this issue Oct 31, 2024
pedro-psb added a commit to pedro-psb/pulpcore that referenced this issue Nov 6, 2024
DRF serializers.JSONField can be any json entity, but we want more precise types
for better schema/bindings representation. New fields that are supposed to be dict
or list structures should use the new JSONDictField or JSONListField field.

Some context: <pulp/pulp_rpm#3639>
pedro-psb added a commit to pedro-psb/pulpcore that referenced this issue Nov 7, 2024
DRF serializers.JSONField can be any json entity, but we want more precise types
for better schema/bindings representation. New fields that are supposed to be dict
or list structures should use the new JSONDictField or JSONListField field.

Some context: <pulp/pulp_rpm#3639>
pedro-psb added a commit to pedro-psb/pulpcore that referenced this issue Nov 7, 2024
DRF serializers.JSONField can be any json entity, but we want more precise types
for better schema/bindings representation. New fields that are supposed to be dict
or list structures should use the new JSONDictField or JSONListField field.

Some context: <pulp/pulp_rpm#3639>
pedro-psb added a commit to pedro-psb/pulpcore that referenced this issue Nov 7, 2024
DRF serializers.JSONField can be any json entity, but we want more precise types
for better schema/bindings representation. New fields that are supposed to be dict
or list structures should use the new JSONDictField or JSONListField field.

Some context: <pulp/pulp_rpm#3639>
pedro-psb added a commit to pulp/pulpcore that referenced this issue Nov 22, 2024
DRF serializers.JSONField can be any json entity, but we want more precise types
for better schema/bindings representation. New fields that are supposed to be dict
or list structures should use the new JSONDictField or JSONListField field.

Some context: <pulp/pulp_rpm#3639>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

No branches or pull requests

6 participants