Argument "id" has invalid value 1. Expected type "ID", found 1. about crystal HOT 20 CLOSED

For others like me that don't know GraphQL well, this is what I think you mean:

query { 
  fooNodes {
    nodes {
      id
      name
    }
  }   
}

Which gives:

{
  "data": {
    "fooNodes": {
      "nodes": [
        {
          "id": "Zm9vOjE=",
          "name": "first foo"
        },
        {
          "id": "Zm9vOjI=",
          "name": "second foo"
        }
      ]
    }
  }
}

Is there a way currently to query by the foo.id column?

from crystal.

Yep, that's it. To get the raw id query the rowId field. It's not a perfect solution, but the id field is required by Relay. So just do the following:

query { 
  fooNodes {
    nodes {
      id
      rowId
      name
    }
  }   
}

How do you feel about this decision? Do you have an idea for a better solution? What do you think about the following:

query { 
  fooNodes {
    nodes {
      id(global: false)
      name
    }
  }   
}

from crystal.

It's easier to specify in the type system. It's easy to have a single required argument, but you can't have in the type system one argument or another. So a signature like this:

foo(id: ID, rowId: Int): Foo

Could accept any of the following:

{
  foo(id: "…")
  foo(rowId: 2)
  foo(id: "…", rowId: 2)
  foo()
}

But two functions with signatures:

foo(id: ID!)
fooByRowId(rowId: Int!)

Is a better description of what we want in the type system.

Also, if you are interested in implementing this feature it should be a good first issue as I this was the initial implementation of foo and similar fields. See createSingleQueryField.js at an earlier commit.

from crystal.

id in this case is a globally unique string identifier created by PostGraphQL, to get this string run fooNodes first. In the future I may add a fooByRowId field.

I should probably clarify this in the docs.

from crystal.

I'm trying to figure out how to find foo where id = 1. I've tried a few things but no luck so far. I don't think I'm qualified to make recommendations about the global thing. Hopefully I will be soon :)

from crystal.

I'll add a fooByRowId field at some point to do this, for now id is just a base64 encoded string which is formatted as {table}:{id}, so you would just base64 encode foo:1.

from crystal.

Ouch, that's a huge gotcha :)

Good I had the idea of reporting a bug or seeing if someone already had. I was quite sure it was something special.

from crystal.

Yeah, it would be great to be able to do:

query {
  foo (rowId: 1) {
    id
    rowId
    name
  }
}

I hope to submit a PR later today if I can figure it out. We'll see.

from crystal.

@rattrayalex nice! I think the best implementation would be to add a new field, fooByRowId which wouldn't be ambiguous with foo.

from crystal.

Interesting - why that approach? (I'm not advanced with GraphQL at this point)

from crystal.

Great, thanks! Hope to give it a shot soon.

On Fri, Apr 29, 2016, 09:25 Caleb Meredith [email protected] wrote:

It's easier to specify in the type system. It's easy to have a single
required argument, but you can't have in the type system one argument or
another. So a signature like this:

foo(id: ID, rowId: Int): Foo

Could accept any of the following:

{
foo(id: "…")
foo(rowId: 2)
foo(id: "…", rowId: 2)
foo()
}

But two functions with signatures:

foo(id: ID!)
fooByRowId(rowId: Int!)

Is a better description of what we want in the type system.

Also, if you are interested in implementing this feature it should be a
good first issue as I this was the initial implementation of foo and
similar fields. See createSingleQueryField.js
https://github.com/calebmer/postgraphql/blob/e61a7cde300e0a0c975b1ba61a115c5d4ceac08b/src/graphql/query/createSingleQueryField.js#L33-L39
at an earlier commit.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#16 (comment)

from crystal.

Looking at https://facebook.github.io/relay/docs/graphql-object-identification.html#content, I don't see a hard requirement that top-level fields provide a global id. That is, users(id: ID!) doesn't appear required by relay; only node(id: ID!), which seems functionally equivalent for Relay's purposes. All Nodes must provide a global id field, so the bothersome-but-acceptable rowId/idmust remain.

If users(id: ID!) exist for convenience, perhaps they could be replaced by non-relay-oriented fields? Relay is awesome and I can't wait to use it with this project, but it might be nice to have a parallel-"relay-free" track that's more convenient to use.

For example:

users(name: "Bob" companyId: 2) {
  id
  rowId
  name
  company {
    rowId
    name
    employees {
       name
    }
  }
}

might return:

{
  "users": [
    {
      "id": "asdjfasdlkjfas=",
      "rowId": 1,
      "name": "Bob",
      "company": {
        "rowId": 2,
        "name": "A Company of Bobs",
        "employees": [
          { "name": "Bob" },
          { "name": "Bob" },
          { "name": "Robert" },
        ]
      }
    },
    {
      "id": "lkjljsdflsdkfj=",
      "rowId": 2,
      "name": "Bob",
      "company": {
        "rowId": 2,
        "name": "A Company of Bobs",
        "employees": [
          { "name": "Bob" },
          { "name": "Bob" },
          { "name": "Robert" },
        ]
      }
    }
  ]
}

Note that the above does away with edges, nodes, etc, as well.

Does that make sense? Thoughts?

(This suggestion is based on the idea that the purpose of fields like foo(id: ID!) is purely for developer convenience, rather than to satisfy a technical/Relay req; absolutely disregard if I'm mistaken)

from crystal.

I have thought about adding a userList or postList field that does this, but I'm hesitant to adopt a field that is functionally equivalent to another. A signature of foo(id: ID!) follows the convention set forth by the SWAPI example, and ideally users just reference nodes using the single global ID instead of primary keys. It's also the only current way to select a single row and postNodes does not allow one to select by an ID.

I also think a postList field is undesirable because it removes pagination context like that from pageInfo and totalCount. I'm also hesitant to add such a field from a technical debt perspective.

A field like fooByRowId provides new functionality as it allows selection of a single node by other means.

from crystal.

I agree PostGraphQL should be ideal/useable for people not using Relay, I just happen to think that design choices made by Relay are helpful for everyone using GraphQL. If you really think node/edges/pageInfo/totalCount really does detract from the developer experience we should consider another field.

(As a side note nodes is actually a convinience field, only edges is required by Relay 😉)

from crystal.

I just happen to think that design choices made by Relay are helpful for everyone using GraphQL.

I agree; I certainly wouldn't want to get rid of them, but I do want to make them optional. In production, I'd generally prefer to use fooNode with the full power of Relay; when playing around & prototyping (which is, as I understand it, a major purpose of this library), I don't want to deal with that. Having both options would be great.

If you really think node/edges/pageInfo/totalCount really does detract from the developer experience we should consider another field.

That sounds great to me. I do like the idea of fooList(rowId: Int, ...other fields). What about also having fooSingle(id: ID, rowId: Int, ...other fields) that raises an error if the number of results is !== 1 ? fooByRowId seems pretty inconvenient, especially considering there are often other fields that I may expect to be unique, and I can't define fooByNameSlug, say. (This could be done with only fields that are UNIQUE NOT NULL).

Again, I view fooList() and fooSingle() to largely be conveniences for quickly playing around with a GraphQL view of one's DB, both in Graphiql and code; I too would encourage most developers to switch over to the Relay fooNode() versions when code is ready for prod.

This kind of exploration seems to me a primary goal of PostgraphQL, but you're the author so the goals are up to you 😉

As a side note nodes is actually a convenience field

I was referring to the top-level node(id: ID!) field, which I believe (perhaps erroneously) Relay uses to lookup items it knows about.

from crystal.

Whoops, forgot a few:

A signature of foo(id: ID!) follows the convention set forth by the SWAPI example

Is that important? I'm not sure how intentional that choice was, or how widespread its adoption may be.

ideally users just reference nodes using the single global ID instead of primary keys.

Are global ID's for people, or machines? Primary keys are much more dev-friendly; if I have to base64 encode foo:id every time I want to look at an object in Graphiql, I'm probably going back to REST 😉

from crystal.

Well thought out points.

That sounds great to me. I do like the idea of fooList(rowId: Int, ...other fields)

I'm really not super excited about copying all of fooNodes arguments into another fooList and I don't think the exclusion of such a field actually hurts players/prototypers. As long as we have good documentation on the nodes and edges fields I don't think it's a combing argument that we need a plain list field.

I think the standard way PostGraphQL should return collectiond is by connections. It's not hard to work with after the initial learning bump and comes with great benefits.

What about also having fooSingle(id: ID, rowId: Int, ...other fields) that raises an error if the number of results is !== 1 ?

The difficulty with a fooSingle field which takes multiple args is what happens for compound keys? I prefer a seperate field for each unique constraint to leverage the type system and better support compound unique columns. Example fields could be:

foo(id: ID!)
fooByRowId(rowId: Int!)
fooByBarAndBaz(bar: String!, baz: String!)

I realize now that I may have been miscommunicating when only saying fooByRowId, I meant any unique constraints get their own field 😊

Are global ID's for people, or machines? Primary keys are much more dev-friendly; if I have to base64 encode foo:id every time I want to look at an object in Graphiql, I'm probably going back to REST 😉

Good point. That's why I want to see the fooBy* fields implemented. It's not just good DX but could also be super helpful.

In summary I'm +1 unique constraint fields and -1 a plain list field as while I do get the DX reasons, we'd just be duplicating logic and forcing devs to make a choice we already know the answer to.

from crystal.

As long as we have good documentation on the nodes and edges fields I don't think it's a combing argument that we need a plain list field.

Well, as a user I'm bummed, but not going to cry 😸 it would be great to make clear, though, that this is primarily a Relay project in the README if that's the direction you want to take.

The difficulty with a fooSingle field which takes multiple args is what happens for compound keys?

They're queried against if provided? I don't think fooSingle(args...) would need to query on unique fields only; just like many ORM's provide, say, .get() instead of .filter() (in Django's case), you could do fooSingle(bar: 2, baz: "apple") and whether the constraint is there or not, you'll get a result iff there's exactly one foo with bar=2 and baz="apple". Is that starting to get a little smelly? Perhaps. But, it's simple, it does what you expect, and makes it super easy for a dev to get a single item.

Now, also having fooByRowId(rowId: ID!) and fooByBarAndBaz(bar: int! baz: str!) could also make sense. And it sounds somewhat fun to write, so I'm not opposed to contributing that 😃

from crystal.

I’m going to close this since there hasn’t been a lot of activity. I’m super for fields that select using table unique indexes (so fooByRowId and fooByBarAndBaz as we discussed). This seems like an easy enough feature to implement if anyone wanted to be added as a collaborator on the project!

from crystal.

The fooByRowId feature we discussed here is being implemented in #70.

from crystal.