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 List Query name changed #8350

Closed
4 tasks done
lawmicha opened this issue Oct 3, 2021 · 13 comments
Closed
4 tasks done

GraphQL List Query name changed #8350

lawmicha opened this issue Oct 3, 2021 · 13 comments
Labels
bug Something isn't working graphql-transformer-v1 Issue related to GraphQL Transformer v1

Comments

@lawmicha
Copy link
Member

lawmicha commented Oct 3, 2021

Before opening, please confirm:

  • I have installed the latest version of the Amplify CLI (see above), and confirmed that the issue still persists.
  • I have searched for duplicate or closed issues.
  • I have read the guide for submitting bug reports.
  • I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.

How did you install the Amplify CLI?

npm

If applicable, what version of Node.js are you using?

v12.19.0

Amplify CLI Version

6.1.1

What operating system are you using?

Mac

Amplify Categories

api

Amplify Commands

add, codegen, push

Describe the bug

Issue

GraphQL Transform creates a different list query name in 6.1.1 as compared to 5.0.1.

As a developer, using Amplify.API from iOS, they are provided with builder methods to create the GraphQL requests:

Amplify.API.query(request: .list(Todo.self))

This request is now failing with the latest CLI provisioning. The overall flow:

  1. Developer defines an input schema:
type NoteData
@model
{
    id: ID!
    name: String!
    description: String
    image: String
}

type Wishes @model 
@key(name: "byUsers", fields: ["idUser"]) 
{

  id: ID!
  date: AWSDateTime!
  idUser: ID!
  ev: ID! 
  deprecated: Boolean!  
  owner: String         
}

type Wish @model 
@key(name: "byUsers", fields: ["idUser"]) {

  id: ID!
  date: AWSDateTime!
  idUser: ID!
  ev: ID! 
  deprecated: Boolean!  
  owner: String         
}

You can ignore most of the details here like the @key usage and the fields. They are from investigating the two issues: Developers following a tutorial from AWS and no longer works. AWS Tutorial. iOS Issue raised by Sebastien. Developer's Amplify.API list query fails. Developer calls out that they are using CLI 5.5.0: iOS Issue

  1. amplify add api, choose API key for simplicity. amplify push, then amplify codegen models

  2. Observed the list query names in AppSync console

with 5.0.1

  • type NoteData -> query listNoteDatas
  • type Wishes -> query listWishess
  • type Wish -> query listWishs

The resulting list query is observed to be generated as: "list" + typeName + "s".

Amplify iOS library aligns with this by having the same logic when generating the query name

/// The GraphQL directive name translated from a GraphQL query operation and model schema data
    func graphQLName(queryType: GraphQLQueryType) -> String {
        let graphQLName: String
        switch queryType {
        case .list:
            graphQLName = (queryType.rawValue + name).pluralize()

/// Appends "s" to the end of the string to represent the pluralized form.
    public func pluralize() -> String {
        return self + "s"
    }

with 6.1.1

  • type NoteData -> query listNoteData
  • type Wishes -> query listWishes
  • type Wish -> query listWishes

Notice that the logic "list" + typeName + "s" no longer holds true. We do have pluralName of the type in the swift schema file after running amplify codegen model, so we can update this logic to "list" + pluralName" to make it work.

Note: If you try to provision the above input schema, you actually have do them separately because type Wishes and type Wish in one schema will fail since they generate the same name:

Error: Object type extension 'Query' cannot redeclare field listWishes

Impact Discussion

1. New projects using latest CLI cannot use Amplify iOS library.

Amplify library needs an update that support the latest CLI version. From the investigation, I believe the library needs to use the "list" + pluralName" and the API calls will start working. Can you verify that this is the correct change to make by identifying the changes in CLI? Should there be a feature flag for this? One of the issues from the developer indicated that they were using CLI 5.5.0.

Looks like #7258 ?

2. Existing developers cannot upgrade to a fixed version of the iOS library

The second problem with this issue is that the identified fix in the library cannot be written in a way that is backwards compatible for existing APIs. amplify codegen models process is not versioned. It doesn't indicate the CLI version in which it was generated with. For example, if it did, then we are able to abstract this upgrade away from developers to some logic that checks the CLI version

if schema.CLIversion < 5.5.0 {
    graphQLName = `"list" + typeName + "s"`
else {
    graphQLName = `"list" + pluralName"`
}

Without the ability to be backwards compatible, developers have to upgrade iOS library with the fix and update the provisioned API (by upgrading the CLI and running amplify push) at the same time. This isn't at all that easy if there are not changes to the schema, amplify status shows "No Change".

3. Are other platforms affected?

Although the details are specific to iOS, you can imagine similar scenarios to other platforms. We should have an issue for each platform to verify this use case is still working or describe the details as to how it works differently from iOS and is not affected.

References

This issue is not related to #7817 since it's for the v2 transformer

@lawmicha
Copy link
Member Author

lawmicha commented Oct 3, 2021

@attilah RE: #4224 (comment)

what you see is by design as the initial pluralization was just to append an 's' and no external libraries was used. This cannot be fixed without breaking existing customer deployments, once we've feature flags implemented it will be a trivial fix.

what is the feature flag with the fix? Do we need the same feature flag in the library that corresponds to the CLI's feature flag so that developers can upgrade both at the same time?

Update: https://github.com/aws-amplify/amplify-cli/pull/7258/files#r720912006

@sebsto
Copy link

sebsto commented Oct 4, 2021

my workaround at the moment is to comment out the .pluralize()call at line 19 here
https://github.com/aws-amplify/amplify-ios/blob/v1.14.0/AmplifyPlugins/Core/AWSPluginsCore/Model/Support/ModelSchema%2BGraphQL.swift#L19

@josefaidt josefaidt added graphql-transformer-v1 Issue related to GraphQL Transformer v1 pending-triage Issue is pending triage labels Oct 4, 2021
@josefaidt
Copy link
Contributor

Hey @lawmicha 👋 thank you for taking the time to raise this with details! Using the posted example I was able to observe the noted error. However, I do see where this can cause further issues.

If attempting to use the following schema:

type Wish @model {
  id: ID!
  name: String!
  description: String
}

type Wishes @model {
  id: ID!
  name: String!
  description: String
}

We'll receive the following error when attempting to push or run amplify api gql-compile

Object type extension 'Query' cannot redeclare field listWishes
An error occurred during the push operation: Object type extension 'Query' cannot redeclare field listWishes

Which, although not explicitly noting there is a collision with the two models' list queries, we are not able to proceed with a push.

what is the feature flag with the fix?

The feature flag to turn off this pluralization is features.graphqltransformer.improvepluralization in your project's amplify/cli.json file.

Marking as a bug 🙂 and looks like we need to:

  1. document the new feature flag, improvedpluralization
  2. improve error output when collisions are detected

@josefaidt josefaidt added bug Something isn't working and removed pending-triage Issue is pending triage labels Oct 4, 2021
@lawmicha
Copy link
Member Author

lawmicha commented Oct 4, 2021

hey @josefaidt, thanks for taking a look. I'm still trying to figure out how to solve the following:

  • New projects using latest CLI cannot use Amplify iOS library because the flag is true by deafult. iOS library constructs the name as "list" + typeName + "s", not "list" + pluralName.

  • If the library fixes this, existing developers cannot take an upgrade to the iOS library because their API is provisioned with the flag set to false.

@lawmicha
Copy link
Member Author

lawmicha commented Oct 4, 2021

Thanks @yuth ! Latest on this is..

amplify codegen models can generate schema with listPluralName and syncPluralName

schema
   @deprecated pluralName
   listPluralName
   syncPluralName

Library code can use this information if it exists, otherwise, fallback to pluralName

code
    list : graphQLName = listPluralName ?? name + "s"
    sync: graphQLName = syncPluralName ?? pluralName ?? name + "s"
  • new developers with the feature flag enabled will model gen the schema with listPluralName and syncPluralName
  • existing developers will continue to use pluralName for sync query and modelName + "s” for list query. When they upgrade, the model gen will generate only listPluralName and syncPluralName.

@lawmicha
Copy link
Member Author

lawmicha commented Oct 4, 2021

@josefaidt can we set improvepluralization to false for new developers until the necessary codegen and library changes are in? This "stops the bleeding" and instead of telling developers to update their cli.json to make things work, we can tell them to upgrade the CLI version. Developers that are looking for this feature can set it to true as per feature flag documentation.

@maziarzamani
Copy link

When do we expect a fix for this? When I create a model named Child it ends up as a listChildren in AppSync however iOS wants to call listChilds.

@johnpc
Copy link
Contributor

johnpc commented Oct 9, 2021

Hey @lawmicha I am a bit confused about this issue report - hopefully you can provide clarification.

First, it looks like there are two issues described here.

The first is that, if you create two models, one called Wish and one called Wishes, there is a Error: Object type extension 'Query' cannot redeclare field listWishes error when attempting to create graphql queries for these fields.

In this case, what is your reasoning for having these two models, and what do you expect the resulting graphql list queries to be? In CLI v5.0.1 the queries would not conflict because they were generated as listWishs and listWishess. But those seem like the wrong query names to me since they are grammatically incorrect. It is frustrating for me when a library I use forces me to include misspellings in my source code! So starting in 5.0.2, by default they both pluralize to listWishes. Grammatically correct, but the name conflict is unavoidable. I would consider why you need a separate model to store a collection of wishes. And if the model truly is required for your use case, perhaps WishCollection or WishList is a better model name.

The second is that code in the iOS library uses a pluralization scheme that mismatches with the pluralization scheme used to create the functioning graphql queries. By the sounds of it, this would impact any app with a model like Wish that pluralizes without just adding an s. I imagine swift code being injected into customer projects that calls a function listWishs that doesn't exist causing compile errors until the developer manually fixes the spelling.

I am not sure how this issue manifests. I tried creating an iOS app with a model called Wish, but the generated API.swift source code used proper pluralization (that is, it created a function called listWishes). The app I created seems to work without issues (following the Getting Started tutorial).

Side note on statements like "Existing developers cannot upgrade to a fixed version of the iOS library" and "New projects using latest CLI cannot use Amplify iOS library." I see there's conversation above on feature flags and how they work. Do you still feel that either or both of these statements are true? If they are, it sounds like no one has been able to develop iOS apps using amplify in months. I want to better understand the scope of the problem here.

@lawmicha
Copy link
Member Author

I agree the first issue related to having two models that end up with conflicting resolver names is unavoidable. I only used them as part of the investigation from the customer issues and to understand the behavior of the CLI. Because of the conflicts, i had to create two separate projects to see what the pluralization logic is for both of them.

For the second issue, I understand it may be a bit confusing to connect the dots since the CLI flow asks to generate an API.swift which is used for AppSync SDK that we are not actively working on. Amplify.API documentation follows amplify codegen models to generate model files and CRUD operations on those models from https://docs.amplify.aws/lib/graphqlapi/query-data/q/platform/ios/ which will not work with the new pluralization.

Both statements are still true because even with the feature flag, we are lacking the merged codegen changes that provide the additional information about what is a list query name, so that it can be used as the source of truth in the fixed library code. The scope is only impacting Amplify.API users using the list query

@cjihrig
Copy link
Contributor

cjihrig commented Nov 29, 2021

I'm going to close this out, as I believe it was addressed a while back on the CLI side. If this should remain open, please let me know.

@cjihrig cjihrig closed this as completed Nov 29, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working graphql-transformer-v1 Issue related to GraphQL Transformer v1
Projects
None yet
Development

No branches or pull requests

7 participants