aws-amplify / amplify-codegen

Amplify Codegen is a JavaScript toolkit library for frontend and mobile developers building Amplify applications.
Apache License 2.0
60 stars 61 forks source link

Swift non model type properties are internal by default #414

Open ethan021021 opened 2 years ago

ethan021021 commented 2 years ago

Before opening, please confirm:

How did you install the Amplify CLI?

npm

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

v14.19.0

Amplify CLI Version

8.0.1

What operating system are you using?

Mac

Amplify Codegen Command

codegen models

Describe the bug

When generating app sync .swift files for non model types, properties on the generated struct are internal by default. I don't think this should be the case as Amplify docs state creating a non model type for S3Object's is the way to properly upload / fetch from S3 using DataStore or the GQL API. Data is accessible by adding a public attribute to the property but it's a pain to constantly add back public after codegen. Instead if it's an S3Object or non model object let's make these properties public by default the struct already is public why shouldn't the properties?

Expected behavior

non model types should have properties that are set to public so developers can access the data the struct holds. If all non model types shouldn't have publicly accessible properties maybe only add public to the properties for S3Object type's only.

Reproduction steps

  1. Add a S3Object type to graphql schema mentioned here: https://docs.amplify.aws/sdk/storage/graphql-api/q/platform/ios/#client-code
  2. run amplify codegen models
  3. Notice the S3Object struct properties do not have a public access control modifier

GraphQL schema(s)

```graphql # Put schemas below this line type Picture @model { id: ID! name: String owner: String visibility: Visibility file: S3Object createdAt: String } type S3Object { bucket: String! region: String! key: String! localUri: String! mimeType: String! } enum Visibility { public private } ```

Log output

``` # Put your logs below this line ```

Additional information

Function that generates non model types:

generateNonModelType() {
        let result = [...this.imports, ''];
        Object.entries(this.getSelectedNonModels()).forEach(([name, obj]) => {
            const structBlock = new swift_declaration_block_1.SwiftDeclarationBlock()
                .withName(this.getModelName(obj))
                .access('public')
                .withProtocols(['Embeddable']);
            Object.values(obj.fields).forEach(field => {
                const fieldType = this.getNativeType(field);
                structBlock.addProperty(this.getFieldName(field), fieldType, undefined, 'DEFAULT', {
                    optional: !this.isFieldRequired(field),
                    isList: field.isList,
                    variable: true,
                    isEnum: this.isEnumType(field),
                    listType: field.isList ? swift_declaration_block_1.ListType.ARRAY : undefined,
                });
            });
            result.push(structBlock.string);
        });
        return result.join('\n');
    }

structBlock.addProperty(this.getFieldName(field), fieldType, undefined, 'DEFAULT', {

Should be:

structBlock.addProperty(this.getFieldName(field), fieldType, undefined, 'public', {

Thank you!

lawmicha commented 2 years ago

Hi @ethan021021, the use of S3Object has not been explicitly tested by the new Amplify DataStore libraries so currently the experience you will face will be sub-par to using AppSync SDK if you decide to use S3Object in your schema. For example, the doc you linked https://docs.amplify.aws/sdk/storage/graphql-api/q/platform/ios/#client-code calls out that the SDK uploads the file to S3 for you, and the following code snippet makes use appSyncClient.perform(mutation: CreatePictureMutation(input: pictureMutationInput)). So if you decide to run amplify codegen models on this schema, it will produce the S3Object swift struct as an Embeddable and have no other functionality such as uploading the S3Object to S3 when using DataStore. We do have plans to support S3Object from DataStore in the future.

In the meantime, i'd suggest naming S3Object to something else just because I'm not sure whether provisioning a backend with the transformer usingS3Object has any special meaning, if using amplify codegen models and DataStore, rename it to StorageObject for example, will still have the same meaning for you, and will still be generated as a "non-model" embedded type of the Picture model. Then you'll have to upload your images using Amplify.Storage and save the data in your Picture model though Amplify.DataStore.

Let me loop back to the team regarding the embedded non-model type's properties being internal and get back to you.

lawmicha commented 2 years ago

Considering Picture gets generated as the struct and properties both being public and that if these types were being placed in its own module, the consuming module woud see the public Embeddable type but not its properties. This definitely looks like a bug. Thanks for the details and additional information containing the fix for this

ethan021021 commented 2 years ago

@lawmicha No problem! So what is the best path forward? Since the data is available in memory on the S3Object when adding a public access modifier I think just adding public to the properties would fix this. And FWIW the S3Object seems to be working with Swift/iOS using DataStore which is great! But I haven't tried a .save operation yet on the iOS side with S3Object i've been only fetching.

lawmicha commented 2 years ago

I believe localUri: "uriOfImageToUpload" can be definitely be set on the embedded type, and then stored on the Picture model, but once you call .save, it won't do anything other than save it to the local DB and sync that data AppSync. You would have to call Amplify.Storage.upload(key, file) and then once the data has been saved successfully, persist the Picture instance containing the information about that uploaded file.

Can you clarify what you mean by fetching S3Object? Is it Amplify.DataStore.query? does that mean you've been saving it from else through other apps on other platforms?

ethan021021 commented 2 years ago

I'm using Amplify.DataStore.observeQuery. Good call on changing S3Object to StorageObject the exact same behavior would occur due to the codegen generating it as a non model / embeddable type. And yes I have a react app that uploads the image, creates the S3Object then save's it on the DataStore's model. I then use a iOS application to observe the query and get the key from the S3Object type. But the issue at hand is key uses the internal file modifier and can't be accessed publicly.

ethan021021 commented 2 years ago

Any movement on this? Thank you!

ethan021021 commented 1 year ago

Hey all,

Starting to use more and more custom object types. Would love for this to go out in a release since I'm getting tired of having to set all these struct properties to public :p thanks!

ethan021021 commented 1 year ago

Hey all,

Another bump as this issue is now more than a year old. Can someone please take a look? Thanks!

ethan021021 commented 5 months ago

Hey all,

Any updates here? Thanks

ethan021021 commented 4 months ago

Any updates….?

marcoboerner commented 1 month ago

Is anything happening in that regard? I moved all my generated models into their own package, and even though the structure of the custom type is marked as public, the individual parameters are not. The following model definitions ...

MyCustomType: a.customType({
  userProfileId: a.id().required(),
  status: a.enum(['new', 'seen'])
}),

... still only generates the following Swift code:

public struct MyCustomType: Embeddable {
  var userProfileId: String
  var status: MatchStatusStatus?
}

desired is:

public struct MyCustomType: Embeddable {
  public var userProfileId: String
  public var status: MatchStatusStatus?
}