chore(api)!: Model .fromJson() Refactor #3032

Equartey · 2023-05-11T19:58:24Z

Issue #, if available:
#816

Description of changes:
Flatens the nested data structure of serializedData across the method channel and the .fromJson() method found on codegen models. This PR is to be paired with changes in aws-amplify/amplify-codegen#593

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

packages/amplify_core/lib/src/types/datastore/conflict_handler/datastore_conflict_handler.dart

...rc/main/kotlin/com/amazonaws/amplify/amplify_datastore/types/model/FlutterSerializedModel.kt

packages/amplify_datastore/example/integration_test/utils/setup_utils.dart

packages/amplify_datastore/example/lib/main.dart

HuiSF · 2023-05-12T17:03:52Z

packages/amplify_datastore/example/lib/main.dart

@@ -5,11 +5,12 @@ library sample_app;

 import 'dart:async';

+import 'package:amplify_core/amplify_core.dart';


I think we should import from amplify_flutter to ensure platform stuff can be correctly configured.

Yes, it's required. amplify_core should only be imported directly in Dart-only environments.

Didn't mean to include this file in this PR, will be removed 👍

...rc/main/kotlin/com/amazonaws/amplify/amplify_datastore/types/model/FlutterSerializedModel.kt

dnys1

Few comments - overall looking good

packages/amplify_core/lib/src/types/datastore/hub/hub_event_element.dart

packages/amplify_core/lib/src/types/datastore/models/subscription_event.dart

packages/amplify_datastore/example/lib/models/BelongsToChildExplicit.dart

dnys1 · 2023-05-13T00:33:14Z

packages/amplify_datastore/example/lib/models/Blog.dart

-                .map((e) => Post.fromJson(
-                    Map<String, dynamic>.from(e['serializedData'])))
+        _posts = json['posts'] != null
+            ? (json['posts']['items'] as List)


Is items guaranteed to be there if posts is non-null?

You could add a safeguard if not:

Suggested change

? (json['posts']['items'] as List)

? (json['posts']['items'] as List? ?? const [])

AppSync will pass an empty items array when posts is specified in the document. When posts is not in the document and therefore the response too, decoding returns null for posts.

dnys1 · 2023-05-13T00:34:18Z

packages/amplify_datastore/example/lib/main.dart

@@ -5,11 +5,12 @@ library sample_app;

 import 'dart:async';

+import 'package:amplify_core/amplify_core.dart';


Yes, it's required. amplify_core should only be imported directly in Dart-only environments.

packages/amplify_datastore/ios/Classes/types/hub/FlutterHubElement.swift

dnys1 · 2023-05-13T00:39:25Z

packages/amplify_datastore/ios/Classes/types/model/FlutterSerializedModel.swift

-                    result[key] = [
-                        "modelName": nextModelName,
-                        "serializedData": try generateSerializedData(
+                    let dataMap = try generateSerializedData(


Similar comment to Hui's. Instead of merging modelName into the model (which may conflict with already-present data), it may be better to return a wrapped value or use __typename instead which is part of the GraphQL spec.

Was naming the field __modelName the resolution to this?

packages/amplify_datastore/pubspec.yaml

dnys1 · 2023-05-13T00:41:25Z

packages/api/amplify_api_dart/test/test_models/legacy/Blog.dart

@@ -15,10 +15,10 @@ import 'package:meta/meta.dart';

 import 'ModelProvider.dart';

-/** This is an auto generated class representing the Blog type in your schema. */
+/// This is an auto generated class representing the Blog type in your schema.


What are we testing with these "legacy" models?

I believe this is validating older versions of codegen that we still support since not all customers update their models/backends. @ragingsquirrel3 may have more context.

packages/api/amplify_api_dart/test/test_models/utils.dart

Jordan-Nelson

I left a few questions, but overall lgtm.

Jordan-Nelson · 2023-05-23T19:51:26Z

packages/aws_common/test/config/profile_file_parser_test.dart

@@ -180,7 +180,7 @@ name = value''';
    });
    test(r'Windows style line endings are supported.', () {
      const configContents = r'''
-[profile foo]
+[profile foo]


Intentional?

Jordan-Nelson · 2023-05-23T20:20:06Z

packages/amplify_datastore/example/ios/unit_tests/resources/FlutterSerializedModelData.swift

@@ -75,7 +75,7 @@ struct FlutterSerializedModelData {
                            "postalCode": "94115"
                        ]),
                        JSONValue.object([
-                            "line1": "000 Somewhere close",
+                            "line1": "111 Somewhere close",


intentional?

Jordan-Nelson · 2023-05-23T20:22:30Z

...tastore/example/ios/unit_tests/resources/query/api/model_name_with_all_query_parameters.json

-    { "field": "created", "order": "descending" }
-  ]
-}
+{


What are the changes in this file?

Jordan-Nelson · 2023-05-23T20:28:45Z

packages/amplify_datastore/ios/Classes/types/model/FlutterSerializedModel.swift

-                    result[key] = [
-                        "modelName": nextModelName,
-                        "serializedData": try generateSerializedData(
+                    let dataMap = try generateSerializedData(


Was naming the field __modelName the resolution to this?

Equartey · 2024-04-22T19:39:00Z

closing in favor of: #4665

Equartey requested a review from a team as a code owner May 11, 2023 19:58

Equartey mentioned this pull request May 11, 2023

chore(amplify-codegen): dart api model .fromJson() refactor aws-amplify/amplify-codegen#593

Merged

6 tasks

HuiSF reviewed May 12, 2023

View reviewed changes

...rc/main/kotlin/com/amazonaws/amplify/amplify_datastore/types/model/FlutterSerializedModel.kt Outdated Show resolved Hide resolved

dnys1 reviewed May 13, 2023

View reviewed changes

Jordan-Nelson changed the title ~~chore(api): Model .fromJson() Refactor~~ chore(api)!: Model .fromJson() Refactor May 23, 2023

Jordan-Nelson reviewed May 24, 2023

View reviewed changes

chore(api): Model .fromJson() Refactor

4c0d4ae

Equartey force-pushed the chore/api-from-json-refactor branch from 8e99a0c to 4c0d4ae Compare April 1, 2024 16:53

Equartey and others added 2 commits April 4, 2024 11:11

regenerated & formatted models

52cd471

Merge branch 'main' into chore/api-from-json-refactor

ac05bf2

Equartey closed this Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(api)!: Model .fromJson() Refactor #3032

chore(api)!: Model .fromJson() Refactor #3032

Equartey commented May 11, 2023

HuiSF May 12, 2023

dnys1 May 13, 2023

Equartey May 15, 2023

dnys1 left a comment

dnys1 May 13, 2023

Equartey May 15, 2023

dnys1 May 13, 2023

dnys1 May 13, 2023

Jordan-Nelson May 23, 2023

dnys1 May 13, 2023

Equartey May 23, 2023

Jordan-Nelson left a comment

Jordan-Nelson May 23, 2023

Jordan-Nelson May 23, 2023

Jordan-Nelson May 23, 2023

Jordan-Nelson May 23, 2023

Equartey commented Apr 22, 2024

		@@ -5,11 +5,12 @@ library sample_app;

		import 'dart:async';

		import 'package:amplify_core/amplify_core.dart';

	? (json['posts']['items'] as List)
	? (json['posts']['items'] as List? ?? const [])

chore(api)!: Model .fromJson() Refactor #3032

chore(api)!: Model .fromJson() Refactor #3032

Conversation

Equartey commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnys1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jordan-Nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Equartey commented Apr 22, 2024