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

GraphQL attribute types are inconsistent and documentation is incorrect #11472

Closed
4xoc opened this issue Jan 12, 2023 · 16 comments
Closed

GraphQL attribute types are inconsistent and documentation is incorrect #11472

4xoc opened this issue Jan 12, 2023 · 16 comments
Assignees
Labels
pending closure Requires immediate attention to avoid being closed for inactivity severity: low Does not significantly disrupt application functionality, or a workaround is available status: revisions needed This issue requires additional information to be actionable topic: GraphQL type: bug A confirmed report of unexpected behavior in the application

Comments

@4xoc
Copy link

4xoc commented Jan 12, 2023

NetBox version

v3.4.2

Python version

3.10

Steps to Reproduce

This issue ticket covers two problems going hand in hand. First is the incorrect schema documentation in GraphiQL, the other is incorrect use of types (always string for nummeric values)

Incorrect docs:

  1. Use GraphiQL to look into schemas for queries and attributes returned. Everything is a string except a few bools

Incorrect use of types:

  1. query for any object with id

Type missmatch between docs and reality:

  1. query for interface_list with mtu attribute. It is described as string type but int is returned.

Expected Behavior

  1. The Schema in GraphiQL represents the correct attribute type identical to the REST API.

  2. Nummerical values are returned as numbers in the response just like with the REST API.

  3. The Schema in GraphiQL actually describes attributes in the correct type they are being returned in.

Observed Behavior

  1. Schema always uses strings for all non-boolean attributes even though many attributes are clearly numbers (like ids).

  2. Many attributes that could be represented as numbers are strings like ids.

  3. Some attributes actually return ints like mtu on interfaces when docs say it's a string.

@4xoc 4xoc added the type: bug A confirmed report of unexpected behavior in the application label Jan 12, 2023
@jeremystretch jeremystretch added the status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation label Jan 12, 2023
@ryanmerolle
Copy link
Contributor

@4xoc great issue! Would you be able to tackle this?

@4xoc
Copy link
Author

4xoc commented Feb 27, 2023

I'm afraid my knowledge of Python and Django is close to nothing. From looking at things I would suspect this being the main cause. IDs are generally uint64 which may not be part of NumberFilter and thus cause the fallback to string. But this is nothing more than a guess. Not sure how that would explain differences between the docs and the actual returned type though.

@4xoc
Copy link
Author

4xoc commented Feb 27, 2023

Apparently GraphQL doesn't support uint64 by default. One can create a custom scalar which should work for such cases.

@rmanyari
Copy link
Contributor

rmanyari commented Mar 1, 2023

Happy to take a look at this, that being said, this is part of the public facing API and changes to types would imply breaking backwards compatibility (perhaps it will need a minor or even major release?)

I'm new to the project and would love the input from one of the more tenured members or maintainers.

  • How do we typically approach these types of changes and what type of backwards compatibility guarantees we offer to users?
  • Is there appetite for this type of changes ATM?
  • From what I've seen so far the UI only uses the REST API, so this wouldn't involve UI changes, can someone confirm?

Thanks!

@arthanson
Copy link
Collaborator

@4xoc I'm not sure I'm seeing this behavior, I do the following query for introspection:

{
  __type(name: "InterfaceType") {
    name
    fields {
      name
      type {
        name
        kind
      }
    }
  }
}

and get back for the mtu field:

        {
          "name": "mtu",
          "type": {
            "name": "Int",
            "kind": "SCALAR"
          }
        },

Can you post a query you use and the return (wrong) values?

@arthanson arthanson added the status: revisions needed This issue requires additional information to be actionable label Mar 22, 2023
@4xoc
Copy link
Author

4xoc commented Mar 27, 2023

Yeah I looked at interface_list definition and arguments of it where MTU must be string. An interface object itself returns int correctly. To rephrase point no 3 from observed behavior: filter arguments that are int in the actual object are required to be string. In my mind id being a string is the biggest issue here.

@bluikko
Copy link
Contributor

bluikko commented Apr 7, 2023

I also was wondering about this when starting to move from REST to GraphQL. I believe this issue refers to example like this:
image
Is this the correct example? Note how all the IDs are of type "String" - in fact almost everything is "String" except some Booleans.

@4xoc
Copy link
Author

4xoc commented Apr 10, 2023

Is this the correct example? Note how all the IDs are of type "String" - in fact almost everything is "String" except some Booleans.

That's exactly it. I believe to have seen some other cases in but can't find any example so I may confuse this again with query parameters (which is probably worth fixing as well).

@jeremystretch jeremystretch added the severity: low Does not significantly disrupt application functionality, or a workaround is available label Jun 23, 2023
@arthanson
Copy link
Collaborator

I'm not sure this is a bug, the above screenshot looks like it is from the GraphiQL browser schema pane which shows things like id as "id: [String]" but if you look at the actual schema definition yaml file it is type array of integers, which the GraphiQL is unhelpfully showing as String. From the yaml file:

      - in: query
        name: id
        schema:
          type: array
          items:
            type: integer
            format: int32
        explode: true
        style: form

That does look correct.

@4xoc
Copy link
Author

4xoc commented Aug 7, 2023

The result from graphql certainly is a string and is required to be a string for queries too. I believe that int32 is incorrect too, some time ago all IDs got changed to int64. IDs definitely are represented as strings.

image

@arthanson
Copy link
Collaborator

Okay, makes sense - was getting a bit confused as I was talking about query params instead of the object itself. Probably using type ID (https://graphql.org/learn/schema/#scalar-types) would be the most correct:

"ID: The ID scalar type represents a unique identifier, often used to refetch an object or as the key for a cache. The ID type is serialized in the same way as a String; however, defining it as an ID signifies that it is not intended to be human‐readable."

@jeremystretch jeremystretch removed the status: revisions needed This issue requires additional information to be actionable label Aug 22, 2023
@arthanson arthanson self-assigned this Oct 10, 2023
@arthanson arthanson removed the status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation label Oct 10, 2023
@arthanson
Copy link
Collaborator

arthanson commented Oct 10, 2023

As stated above GraphQL doesn't natively support int64 but you can use scalars, when we move to strawberry #9856 we can replace it with https://strawberry.rocks/docs/types/scalars#int64 marking as blocked by #9856

@arthanson arthanson added the status: blocked Another issue or external requirement is preventing implementation label Oct 10, 2023
@arthanson arthanson removed their assignment Apr 18, 2024
@arthanson arthanson added the status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation label Apr 18, 2024
@arthanson
Copy link
Collaborator

@4xoc I think this is resolved in NetBox 4.x as the new strawberry returns the fields as ID fields "The ID scalar type represents a unique identifier, often used to refetch an object or as key for a cache. The ID type appears in a JSON response as a String; however, it is not intended to be human-readable. When expected as an input type, any string (such as "4") or integer (such as 4) input value will be accepted as an ID." Can you please confirm that this would resolve the issue for you.

@arthanson arthanson self-assigned this May 20, 2024
@arthanson arthanson added status: revisions needed This issue requires additional information to be actionable and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation status: blocked Another issue or external requirement is preventing implementation labels May 20, 2024
@arthanson arthanson assigned arthanson and unassigned arthanson May 23, 2024
@4xoc
Copy link
Author

4xoc commented May 29, 2024

I've not yet had a chance to look into a 4.x installation but looking through the documentation it appears as using ID type scalar solves the issue of supplying integers for queries.

However, on the return side of things it'll still return a string that has to be parsed client side. Looking at the docs it looks to me like maybe using BigInt (https://strawberry.rocks/docs/types/scalars#bigint-64-bit-integers) would be a better way to go. Afaik IDs in Netbox are unsigned 64 bit integers and thus should be returned as such. I don't think Python itself sees a difference as everything is just int so it probably works with bigint out of the box.

Copy link
Contributor

github-actions bot commented Jun 6, 2024

This is a reminder that additional information is needed in order to further triage this issue. If the requested details are not provided, the issue will soon be closed automatically.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Jun 6, 2024
Copy link
Contributor

This issue is being closed as no further information has been provided. If you would like to revisit this topic, please first modify your original post to include all the requested detail, and then ask that the issue be reopened.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 14, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pending closure Requires immediate attention to avoid being closed for inactivity severity: low Does not significantly disrupt application functionality, or a workaround is available status: revisions needed This issue requires additional information to be actionable topic: GraphQL type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

6 participants