Amplify-cli: Connections to Models with range key generate incorrect templates

[RossWilliams]

Describe the bug
Using the @connection directive as a one to one mapping, where the target model has a primary sort key does not generate correct templates.

To Reproduce
with the given model

type Model1 @model @key(fields: ["id", "sort" ]){
  id: ID!
  sort: Int!
  name: String!
}
type Model2 @model {
  id: ID!
  connection: Model1 @connection(sortField: "modelOneSort")
  modelOneSort: Int!
}

the query

query {
  getModel2(id: "1") {
    connection {
      name
    }
  }
}

will cause an error as null is returned for a non-nullable field.

The reqest template for model1.model2.req.vtl will have a getItem with a single "id" key
Expected behavior
connecting to a model with a sort key does not fail

[RossWilliams]

2230 contains a fix for this issue by changing the definition of Model2 to:

type Model2 @model {
  id: ID!
  connection: Model1 @connection(fields: ["modelOneId", "modelOneSort"])
  modelOneId: ID!
  modelOneSort: Int!
}

[nikhname]

Closing since #2230 fixes this

[RossWilliams]

@nikhname #2230 does not fix this, but provides an alternative implementation as a workaround. If connections cannot be made to models with custom primary keys in the main table, then the tool needs to specifically prevent generating these templates, and the documentation needs to state this special case.

Please re-open.

[attilah]

@RossWilliams a fix is in the works, but fixing this issue I discovered an issue around keys when the keys are Int. The issue you reported here is already fixed in the PR, but I did not include it in the list of issues fix, because you used a number for sortKey and $util.defaultIfNullOrEmpty fails on numbers, and we're ending up with a nullified DynamoDB expression, so you'll get back null as result of the query.

[warrenmcquinn]

@attilah Thanks for posting about this - I just added a sortField to a new @connection, with an Int as the key. sortDirection was behaving exactly as you describe.

Is the fix in the works or already implemented in the PR? Thanks 🙌

[attilah]

Fixed in the PR above.

[houmark]

@attilah While I appreciate the attempt to fix this bug, this bugfix seems to have broken our ability to push on 3.14.0+ and there seems to be no way to solve this?!

We have some "old" but active and critical models with a named @connection and sorting on createdAt which have worked just fine until now:

ApplicationAnswer
...
application: Application @connection(name: "Responses", sortField: "createdAt")
...

With this fix, our schema is no longer valid and after having spent 8+ hours trying all kinds of attempts to fix this while downgrading to around 3.10.0 and re-upgrading to 3.14.0, we are in a locked situation where I am out of ideas on how to fix this without dropping my PRODUCTION tables in DynamoDB which is NOT an option.

Removing the sortField in the connection seems not to be viable, as we have many connections and it will only allow one at a time due to DynamoDB limitations? Honestly, I am not sure right now if this is the only potential route, but this was my first attempt and it did not work out, for reasons I no longer fully remember to be honest. But even if we were able to do that, adding the primary key is not possible as described below.

It all started with this error (when doing amplify gql-compile api or amplify push):
InvalidDirectiveError: sortField "createdAt" requires a primary @key on type

This could have been more descriptive, even maybe with a link to the documentation or an example of how that key should be done, but after reading the docs it was clear that our Application model needs a primary unnamed key like (attempts to create secondary indexes with createdAt did not make the compile error go away):

@key(fields: ["id", "createdAt"])

Unfortunately, that just brings us to a new error:

CloudFormation cannot update a stack when a custom-named resource requires replacing. Rename Application-uniquename-dev and update the stack again.

Renaming Application will drop the existing DynamoDB table and all its data!!!

Effectively, and please do correct me if there's something I have missed here, existing models not having an unnamed primary key which has connections that uses a sort field like the above-described situation will be SOL due to this aggressive bugfix that does not consider any type of backward compatibility or any type of migration?!?

Again, I am aware that this was a bugfix for a "non-critical" situation which of course should be fixed, but why did you not consider the side effects of this type of fix? One thing is there's a bug here, but you cannot just fix it without considering how this affects platforms in the wild.

Please advise if I missed something obvious, roll back this bugfix, or create a migration or another way for existing users to get out of this situation.

[RossWilliams]

This is not a full response, and I’m on my mobile, but a few items to note:

1. This should probably be a new issue
2. You have a sortfield on a connection to a single item. Can we assume the other side is a many relationship that also has a sortfield property? If no, then the sortField property serves no purpose and can be removed.
3. If you need to solve your use case with a GSI, you can use the new “keyName” argument on the @connection directive to force the connection to use a GSI.
4. Adding or removing more than one connection at a time per model is now possible using the “keyName” argument. The previous limitation was due to GSI creation per connection, now you can have many connections use the same GSI.
5. When you say there are “old” models, it may help to understand the indexes that are being targeted here. Sharing whether your connections are using the primary table or a GSI would help (look in the connection VTL file).
6. Some more information would help to understand if this is a bug, or whether a parameter was being incorrectly specified but had no side effects until now. Simplified models, gsi names, etc could be shared to find out.

[houmark]

Very good guidelines here @RossWilliams — I will try out some of these and give feedback.

[houmark]

So after messing around again with my schema, and removing some indexes I had created both in the schema and directly in DynamoDB I followed mainly step 2. Turns out I had some sortBy on named connections that were 1 to many but on the other side they would be of course just 1 to 1. And the 1 to 1 seems to be the ones making problems. Remove the sortField there seems to have solved it and the error I was given was simply misleading.

I added a comment on this issue because I seemed related, and I still think it is. And I was hoping for feedback from @RossWilliams - so thanks for your prompt list of potential issues, that did help me a lot.

keyName may be called keyField? It did not seem to apply in my case. I did not need any new indexes or name the ones I had specifically, I just needed to remove some copy/paste configuration (I would copy the named connection to both sides and keep the sortField because it was not an issue in the past).

It's been a few tough days in the attempt to simply update a setup that was up to speed around one month ago. I got hit by no less than three breaking changes which each took some time to figure out. Maybe I'm just ignorant but it definitely did not help that those three bugs were somewhat conflicting with each other and that had me moving versions back and forward in an attempt to work through all of them. I am now on 3.15.0 and even Amplify Console seems to be deploying without issues — for now...

[RossWilliams]

@houmark I looked at the PR resolving this issue and the error you encountered. You do seem to have a 1:1 connection with no sort key on the connecting object, so specifying a sortField would be incorrect and against the documentation. The PR fixed a bug where the sortField was being ignored when it shouldn't have been, and your incorrect use was exposed. The error messages were a nice touch to the PR, but maybe it s

Considering your original question

existing models not having an unnamed primary key which has connections that uses a sort field like the above-described situation will be SOL due to this aggressive bugfix that does not consider any type of backward compatibility or any type of migration?!?

Your connection did not use a sort field, and this is not a breaking change of the specification. I believe the only users this will affect will be those with improperly specified connection parameters.

I'm sympathetic to your situation, directives have optional parameters, and in general specifying unused parameters has no ill effect. The codebase does not lend itself to an easy check for when this happens, so I'm not sure what can be done to prevent this in the future. Perhaps taking care to consider when a previously ignored parameter is used and write more verbose error messages. Or maybe the error message could be updated to:

`sortField "${sortFieldName}" FOR A 1:1 CONNECTION requires a primary @key on type "${relatedTypeName}" with a sort key that was not found. DID YOU MEAN TO SPECIFY A SORT FIELD`

btw, keyName is part of #2071 , a great feature. If you use it for your connections where a GSI is created, you have a lot more flexibility to make atomic changes to your schema (add multiple connections at once, change a connection specification in one step). Its worth going through the pain of converting to it.

[houmark]

Yes, it's correct. The one that was the problem, was 1:1 connection which I missed in the gist of going over them all, mainly due to the error message which first pointed me to Application but the problem was on the other model. If Amplify could show the line number with the problem in the schema, then the error message could be less informative (as it kinda is now) because one thing is to understand the error, the other part is to understand where the error is originating.

I fully accept that I had a bad configuration here, but that was partly because Amplify was too lax on its error checking in the past, so when that's tightened which I fully support, I think it's also important to improve on the error messages and try to make sure it's as elaborate as possible.

I would like to think that I normally would figure this out in minutes or an hour or so, but I had some very long sessions of debugging a total of 3 incidents that happened due to my upgrade, so I was tired and sick of this framework, because I literally cannot remember doing an upgrade without issues since I went with Amplify some 4-5 months ago.

I really appreciate your time to guide me here and I will for sure have a look at #2071 to see if I dare moving to that with the hope of future updates and changes to my schema will be less painful. Thanks for the hint!