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

[codegen models] add 'required' modifier in constructors when a field is non-nullable #665

Open
13 tasks
dJani97 opened this issue Jun 2, 2023 · 8 comments
Open
13 tasks
Assignees
Labels
bug Something isn't working Dart Target language: Dart p2 transferred Issue has been transferred from another Amplify repository

Comments

@dJani97
Copy link

dJani97 commented Jun 2, 2023

Description

The models generated by amplify codegen models won't compile because of the following error:

The parameter 'id' can't have a value of 'null' because of its type, but the implicit default value is 'null'. (Documentation)
Try adding either an explicit non-'null' default value or the 'required' modifier.

The generated field definition looks like this:

final String id;

And the generated constructor looks like this:

const Someclass._internal({this.id});

Since this field is non-nullable, the constructor should be generated like so:

const Someclass._internal({required this.id});

This causes us some overhead as we have to update the generated files manually each time.

We're on CLI version 12.0.3 and Flutter version 3.10.2.

Categories

  • Analytics
  • API (REST)
  • API (GraphQL)
  • Auth
  • Authenticator
  • DataStore
  • Storage

Steps to Reproduce

Have a model with a non-nullable field.

Run amplify codegen models.

Try compiling they project with the latest version of Flutter.

Screenshots

No response

Platforms

  • iOS
  • Android
  • Web
  • macOS
  • Windows
  • Linux

Flutter Version

3.10.2

Amplify Flutter Version

1.1.0

Deployment Method

Amplify CLI

Schema

No response

@Equartey
Copy link
Contributor

Equartey commented Jun 2, 2023

Hey @dJani97, sorry you're having this issue.

Codegen should indeed mark that field as required.

I tried generating a simple model using the same versions as you and codegen marked it required as expected.

Can you provide the schema you're working with?

@Equartey Equartey self-assigned this Jun 2, 2023
@HuiSF
Copy link
Contributor

HuiSF commented Jun 2, 2023

Hi @dJani97 The codegen generated code may unnecessarily conform all linter rules as the comment inserted in the generated file:

// NOTE: This file is generated and may not follow lint rules defined in your app
// Generated files can be excluded from analysis in analysis_options.yaml
// For more info, see: https://dart.dev/guides/language/analysis-options#excluding-code-from-analysis

We recommend you to do so to avoid linter noise.

@Equartey
Copy link
Contributor

Equartey commented Jun 8, 2023

Hey @dJani97, are you still facing this issue?

If so can you please provide your schema? And are you using any feature flags with Amplify CLI?

@dJani97
Copy link
Author

dJani97 commented Jun 12, 2023

@HuiSF
These are not lint warnings, but compilation errors, so I can't just ignore them:

Error: The parameter 'id' can't have a value of 'null' because of its type 'String', but the implicit default value is 'null'.
Try adding either an explicit non-'null' default value or the 'required' modifier.


@Equartey
For example this schema:

type SomeModel {
  id: String
  name: String
}

generates the following model:

@immutable
class SomeModel {
  final String id;
  final String? _name;

  String? get name {
    return _name;
  }
  
  const SomeModel._internal({this.id, name}): _name = name;
  // ...
}

Even though both fields are using the String notation instead of String!, for some reason, Amplify knows that the id: String field should be non-nullable. And so it makes it non-nullable in the field definition, but it doesn't apply the required keyword to the constructor:

image

Now I understand that this could be fixed by updating the model like so:

type SomeModel {
  id: String!
  name: String
}

... but still, this behavior is confusing and if amplify will make all id fields required by default, then it should also make it required in the constructor.


Edit: I consulted with backend, and they say that this field IS actually nullable on their side. So this is why they added it to the schema this way. Why does Amplify CLI make this decision for us, and treat all id fields as non-nullable by default?

@Jordan-Nelson
Copy link
Contributor

@dJani97 - I am not able to reproduce this.

The following schema:

type SomeModel @model {
  id: String
  name: String
}

produces the following generated code:

class SomeModel extends Model {
  static const classType = const _SomeModelModelType();
  final String id;
  final String? _name;

  @override
  getInstanceType() => classType;
  
  @Deprecated('[getId] is being deprecated in favor of custom primary key feature. Use getter [modelIdentifier] to get model identifier.')
  @override
  String getId() => id;
  
  SomeModelModelIdentifier get modelIdentifier {
      return SomeModelModelIdentifier(
        id: id
      );
  }
  
  String? get name {
    return _name;
  }
  
  const SomeModel._internal({required this.id, name}): _name = name;

  // rest of class definition
}

Can you provide the output of running amplify -v and the contents of amplify/cli.json?

@Jordan-Nelson
Copy link
Contributor

Can you also provide the model definition of the model you are having this issue with? I know you provided an example model definition, but seeing the actual definition might help us reproduce this and understand the use case better. Thanks.

@dJani97
Copy link
Author

dJani97 commented Jun 12, 2023

Before writing the previous comment, I added this exact SomeModel definition to the schema and generated it. So all the code I pasted was actual generated code.

I noticed that you're using the @model annotation in your definition, while I was not.

@Jordan-Nelson
Copy link
Contributor

I see. I am able to reproduce that. It seems that codegen assumes that if there is an id field on a non-model type (aka a custom type) that the field will be non-null. I think the type is essentially being treated like a model type (which would require the ID field). I will mark this as a bug.

Possible workarounds:

  • Rename the "id" field as this only seems to be an issue if the field is named "id".
  • Modify the generated code manually

Reproduction steps:

  1. Create a schema that contains a non-model type that has a field named "id". That field should not be required. For example:
type MyModel @model {
  id: ID!
  name: String
  someModel: MyType
}

type MyType {
  id: String
  name: String
}
  1. Run amplify codegen models
  2. Observe that there is a compilation error.

@Jordan-Nelson Jordan-Nelson added bug Something isn't working and removed pending-response labels Jun 13, 2023
@NikaHsn NikaHsn transferred this issue from aws-amplify/amplify-flutter Aug 10, 2023
@Jordan-Nelson Jordan-Nelson added the transferred Issue has been transferred from another Amplify repository label Aug 10, 2023
@AaronZyLee AaronZyLee self-assigned this Aug 18, 2023
@AaronZyLee AaronZyLee added the Dart Target language: Dart label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Dart Target language: Dart p2 transferred Issue has been transferred from another Amplify repository
Projects
None yet
Development

No branches or pull requests

6 participants