aws-amplify / amplify-cli

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development.
Apache License 2.0
2.81k stars 821 forks source link

reusing an existing ID field in a connection leads to error: All fields provided to an @connection must be non-null scalar or enum fields. #3275

Closed AndreasEK closed 3 years ago

AndreasEK commented 4 years ago

Which Category is your question related to? AppSync / GraphQL

Amplify CLI Version 4.12.0

What AWS Services are you utilizing? AppSync, DynamoDB, Cognito, S3

Provide additional details e.g. code snippets Given an existing model:

type Order
  @model
  @key(
    name: "ByUser"
    fields: ["userId", "createdAt"]
    queryField: "ordersByUser"
  )
  id: ID!
  …
  userId: ID
}

Why would the addition of a new connection like this

  user: User @connection(fields: ["userId"])

Lead to the error: All fields provided to an @connection must be non-null scalar or enum fields.

When adding the connection without the fields parameter, the generated orderUserId would have the same properties as the already existing userId, but it would work with the connection.

hirochachacha commented 4 years ago

dup of https://github.com/aws-amplify/amplify-cli/issues/1657

AndreasEK commented 4 years ago

Thanks for linking the other issue. I fail to see the connection between these two, though. Without the new user connetion our "Order" was behaving as expected. 'createdAt' was automatically set on creation, as one would suspect. Isn't that what #1657 is about?

In this issue, I was asking, why adding a connection claims to require non-null scalar fields, while the provided field (userId) has those properties.

hirochachacha commented 4 years ago

Oh, sorry for misreading. If I understand correctly, you pointed out inconsistency between @key and @connection - @key accepts nullable fields, on the other hand @connection rejects nullable fields. That's not dup of #1657.

lenarmazitov commented 4 years ago

I have same issue. I have table which have self connected attribute toId, which can be leaved unset, because there is two entities in table: User and Subscription. If I leave it nullable I have error: All fields provided to an @connection must be non-null scalar or enum fields Otherwise I should set toId attribute for User entity, but it will be better to leave it empty and only remember it when I use Subscription entity.

type Entity
    @model(mutations: null, subscriptions: {level: off})
    @key(fields: ["pk", "sk"]) 
{
    pk: ID!
    sk: String!
    toId: ID
    to: Entity @connection(fields: ["toId"])
}

Before it worked fine, but now I can not have @connection with nullable field.

mdebo commented 4 years ago

@AndreasEK I ask myself the same question: why adding a connection require a non nullable field?

AndreasEK commented 4 years ago

I read the replies so far as a temporary inconsistency, which hopefully will be resolved at some point.

CodySwannGT commented 4 years ago

+1 this is a major oversight and prevents a model from having optional connections

mdebo commented 4 years ago

@CodySwannGT @hirochachacha Yes really it's really painfull to deal with these kind of oversight. Having optional connections is often necessary. I meet the same problem. If I specify the field use in connection it should be non nullable. It works if i dont specify it and the generated field is nullable

ajhool commented 4 years ago

@hirochachacha The #1657 issue is related in the sense that it would resolve our problem but this is a separate issue and not a duplicate. This is simply two undocumented issues that result in a catch-22 and prevent basic usage of this framework.

  1. You can't make a model's primary ID or owner ID non-nullable because then you would need to set it on the client side during object creation, as opposed to leveraging AppSync's autogeneration. (#1657)

  2. You can't use a connection for a primary ID or the owner id if the ID is nullable (this issue).

  3. Thus, you can't use a model's primary ID if you want to leverage AppSync's autogeneration or owner id.

  4. Thus, you can't use appsync autogeneration for ID if your API uses the connection directive.

hirochachacha commented 4 years ago

@ajhool I already stand corrected at https://github.com/aws-amplify/amplify-cli/issues/3275#issuecomment-579203618.

elgutierrez commented 4 years ago

Hey, do we have any update on this issue or any valid workaround? It's an important requirement for my app to have optional connections, and simply using @connection without fields create some other problems for me.

jskrt commented 4 years ago

Bump

vrambo8 commented 4 years ago

I guess a workaround could be having a dummy instance of the type you need to connect to and use it as a default connection in the case in which you would actually need it to be null.

Nonetheless this is a major feature that needs to be implemented ASAP.

mattstobber commented 4 years ago

I was creating a nested tree and need to have optional connections.

tomhirschfeld commented 4 years ago

bump please, just running into this issue here

maximelebastard commented 4 years ago

Same issue here, the API seems inconsistent on that point

blydewright commented 4 years ago

@tomhirschfeld

To get around this issue, explicitly declare the amplify internally generated FK property as optional (make sure to use their naming convention so it maps the data correctly):

orderUserId: ID
user: User @connection

The naming convention is: {camel-cased model name - order}{property name with capital - User}Id

When this property has been explicitly declared, it now becomes available in queries, mutations and filters and can be used in @key directives.

raffibag commented 4 years ago

@tomhirschfeld

To get around this issue, explicitly declare the amplify internally generated FK property as optional (make sure to use their naming convention so it maps the data correctly):

orderUserId: ID
user: User @connection

The naming convention is: {camel-cased model name - order}{property name with capital - User}Id

When this property has been explicitly declared, it now becomes available in queries, mutations and filters and can be used in @key directives.

Can you please link to the documentation where this is explained (provided it exists)? I tried what you proposed and it didn't work.

blydewright commented 4 years ago

@raffibag I didn't find this in any documentation, but figured it out through trial and error. Can you share some code and what you're trying to achieve so I can help?

raffibag commented 4 years ago

Thanks, @blydewright - I may have misunderstood the implications of your solution... I was assuming that if implemented correctly, you'd be able to access the association when calling the parent object. In my case (though I was able to figure out another solution that ended up working better for our project), I had two associations under a Plan model: spark and widget. In my code (under the Plan model) I originally represented these associations as:

sparkId: ID!
spark: Spark @connection(fields: ["sparkId"])
widgetId: ID!
widget: Widget @connection(fields: ["widgetId"])

This, of course, caused the error that's the subject of this thread... So I adopted your strategy and tried:

orderSparkId: ID
spark: Spark @connection
orderWidgetId: ID
widget: Widget @connection

The problem was when I tried creating the associations (using a GraphQL mutation), the parent object (Plan) returns null for the spark and widget objects. Note: I was creating the association only by writing IDs to either the orderSparkId or orderWidgetId fields (not by writing the actual spark or widget models' data to their respective fields).

Again, I may have totally misunderstood the expected outcome of your solution, but that's why I was asking for a link to the docs.

blydewright commented 4 years ago

Hi @raffibag no problem :) We're all trying to figure this out with limited documentation and examples since it's so new!

Glad to hear you sorted out a solution. Perhaps it may help to point out a few things in your example code above:

  1. Your orderSparkId and orderWidgetId fields would only work if the model they reside in is called Order. But based on what you described, they should be planSparkId and planWidgetId as they reside in a Plan model. The rest of your implementation looks good.
  2. Creating/updating by passing the planSparkId or planWidgetId is the only way I know of that will allow the underlying model to persist the change. The internal resolvers (created due to the @connection directive), will take care of building the appropriate widget and spark data for the graphql response payload.
  3. Setting things up in this way (using planWidgetId and planSparkId) is only necessary when needing OPTIONAL relations. Had spark and widget been mandatory at the time of creating the model, you could have stuck to your original sparkId: ID! implementation

Does this help and make sense?

raffibag commented 4 years ago

Absolutely, makes sense @blydewright. Thanks for the explanation! I went back and tried it out and it worked perfectly - was able to get the optional associations to show up simply by writing the respective ID to the parent model. I didn't read the previous posts closely enough and assumed "order" was an Amplify directive (or something like that). Note also that camelcase needs to be carried through all the way (i.e., camelCaseId will work but camelCaseID will not).

Hopefully this gets documented, as my concern now is that any breaking changes will likely not get relayed to those who used this as a workaround / solution.

blydewright commented 4 years ago

Glad it works :) I don't believe that this "workaround" will result in future errors, unless the amplify team change the way they internally name relational IDs which would pretty much break everything ;) We're simply plugging into their naming convention to accommodate optional relations which, in my opinion, should be catered for by amplify when defining your own foreign key fields: i.e. using widgetId: ID (without !) and widget: Widget @connection (fields: ["widgetId"]). That declaration should work but doesn't..

Good luck with your development!

hutber commented 4 years ago

Oh boy, this is a scary situation as any changes down the road will invalidate my current data. Sometimes I really regret using Amplify, other times I think "oh this is seriously cool" But I certainly shouldn't have used it in a production environment !

jimjoes commented 3 years ago

Yep just hit this, too. +1.

blazinaj commented 3 years ago

A dirty way around this is to just provide non-existing IDs for the required ID fields that are null.

createPlan {
  widgetID: "thisWidgetIDneverExists"
  sparkID: "thisSparkIDneverExists"
}

This (surprisingly) does not cause an error. And when you query Plan, the connections will return null.

yaquawa commented 3 years ago

Thanks @blydewright Your solution works for me. me too was wondering what happens if only use @connection without any arguments (https://github.com/aws-amplify/amplify-js/discussions/7593)

The doc given an example like this:

type Project @model {
  id: ID!
  name: String
  team: Team @connection
}

type Team @model {
  id: ID!
  name: String!
}

But after I saw your solution, shouldn't the doc's example code be this???

type Project @model {
  id: ID!
  name: String
  team: Team @connection
  projectTeamId: ID
}

type Team @model {
  id: ID!
  name: String!
}

I'm not should if the doc's example will work(not tried yet), There is no description in the doc.. Hope the Amplify team put this in the docs.. it's so confusing.

blydewright commented 3 years ago

@yaquawa no problem :)

You can use the structure as the docs describe without issue. Internally, a field called projectTeamId will automatically be created for the mapping to exist. The only real purpose for manually declaring that field (in an optional @connection relationship), was if you, like me, want the field to have a specific name so that you can refer to it in some way. I would refer to the field explicitly for defining @keys and/or query filtering or other use cases in my app. From my experience, you can only do this if the field is explicitly declared.

If you don't need to refer to the @connection field in any way, you can just define your @model in the way the docs describe.

yaquawa commented 3 years ago

@blydewright Thanks for your kindly explanation! Hope the team can fix this issue too..!

edwardfoyle commented 3 years ago

Nullable connection fields are now supported in the latest version of the CLI. Please upgrade to try it out!

tomhirschfeld commented 3 years ago

@edwardfoyle yee haw! thanks!

anthonymoretti commented 3 years ago

Nullable connection fields are now supported in the latest version of the CLI. Please upgrade to try it out!

@edwardfoyle I just submitted #558 because in Flutter I'm still having issues with nullable connection fields.

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.