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

Refs #32685 - Keep modulized model name in Global ID #9458

Conversation

kamils-iRonin
Copy link
Member

This is to properly handle model names that are scoped by plugin name. Because the graphql type name can't contain :: we could use _ instead

 module ForemanPuppet
   module Types
     class Environment < ::Types::BaseObject
       graphql_name 'ForemanPuppet_Environment'

however, the Global ID should contain the correct model name, with ::.

[1] pry(main)> Foreman::GlobalId.decode("MDE6Rm9yZW1hblB1cHBldDo6RW52aXJvbm1lbnQtMjgz")
=> [1, "ForemanPuppet::Environment", "283"]

@theforeman-bot
Copy link
Member

Issues: #32685

@evgeni
Copy link
Member

evgeni commented Oct 11, 2022

Is that the same/related to the patch @ofedoren has in #9449?

@kamils-iRonin
Copy link
Member Author

Is that the same/related to the patch @ofedoren has in #9449?

This is a slightly different as it addresses the proper generation of Global ID for models scoped by plugin name. I would merge #9449 first, then rebase and merge this one.

@evgeni
Copy link
Member

evgeni commented Oct 11, 2022

I've merged #9449, go ahead with the rebase :)

@kamils-iRonin kamils-iRonin force-pushed the refs-32685-keep-modulized-model-name-in-global_id branch from 0044739 to 61b8812 Compare October 12, 2022 08:04
@kamils-iRonin
Copy link
Member Author

rebased and all 🟢

@evgeni
Copy link
Member

evgeni commented Oct 12, 2022

This looks correct to me, but my GraphQL knowledge is really limited.

@ofedoren mind having a second look here?

@ofedoren
Copy link
Member

ofedoren commented Oct 13, 2022

Well, I've done something similar, but kidna hidden: 6b557cf

Instead of explicitly call graphql_name for each resource/plugin, my changes would try to do it automatically unless we can directly assign the actual model via graphql_type.

This PR just makes the same, but explicitly. I'm not against this fix, just not sure if it's actually needed.

UPD: I can also be mistaken since my understanding of the library is also poor...

@kamils-iRonin
Copy link
Member Author

This PR just makes the same, but explicitly.

I don't think it's the same. I just tested Global Id generation on the develop branch and used the puppet plugin for testing.

[1] pry(main)> Foreman::GlobalId.decode(Foreman::GlobalId.for(ForemanPuppet::Environment.find(5)))
=> [1, "Environment", "5"]

As you can see, ForemanPuppet namespace is missing in the type name. So, I set graphql_name to ForemanPuppet_Environment (theforeman/foreman_puppet#311) and tested again.

[1] pry(main)> Foreman::GlobalId.decode(Foreman::GlobalId.for(ForemanPuppet::Environment.find(5)))
=> [1, "ForemanPuppet_Environment", "5"]

It's better, but still, I expected the type to be ForemanPuppet::Environment, as it is in Foreman 3.4, before the GraphQL update. And this patch fixes that.

[1] pry(main)> Foreman::GlobalId.decode(Foreman::GlobalId.for(ForemanPuppet::Environment.find(5)))
=> [1, "ForemanPuppet::Environment", "5"]

ofedoren
ofedoren previously approved these changes Oct 17, 2022
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ofedoren is on PTO, but I'm going to trust your code changes.

@ekohl ekohl merged commit 339fe63 into theforeman:develop Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants