feat(amplify-xcode): generate JSON schema #1080

diegocstn · 2021-02-26T18:01:42Z

Description of changes:
This PR introduces a new amplify-xcode genenerate-schema command to generate a JSON representation of the CLI and its commands as an intermediate step to further generate necessary JavaScript/TypeScript bindings.
In order to properly generate bindings and documentation for each commands, an interface describing parameters and their usage is generated according to the following specs:

{
  name: String, // parameter name
  help: String, // brief description
  kind: "option" | "flag" | "argument", // type of parameter (--option=value, --flag, <argument>),
  type: String, // string representation of parameter type, used to generate docs
}

Based on the current set of commands defined in amplify-xcode, the generated JSON schema will look like

{
  "commands":[
     {
        "name":"import-config",
        "abstract":"Import Amplify configuration files",
        "parameters":[{ "kind":"option", "name":"path", "type":"String", "help":"Project base path"}]
     },
     {
        "name":"import-models",
        "abstract":"Import Amplify models",
        "parameters":[{ "kind":"option", "name":"path", "type":"String", "help":"Project base path"}]
     }
  ],
  "abstract":"Auto generated JSON representation of amplify-xcode CLI"
}

The following implementation is the results of a set of constraints/limitations in the ArgumentParser library and Swift runtime.

Using reflection to inspect a command type and generate a schema isn't a viable option due to the following constraints:

ArgumentParser derives the name used on the command line from the name of the properties annotated with @Option @Flag property wrappers by using a private String extension, so for example a property named fileOutputPath will be converted to a file-output-path parameter when used to invoke the CLI
trying to replicate the criteria used to derive parameters names may be inconsistent (how to handle special cases, like numbers and acronyms)
both @Option and @Flag are generics on their wrapped value, that makes casting and/or infer their types using reflection almost impossible
structs/classes used internally by ArgumentParser to parse value and/or generate help messages are private to the library and not exposed to its consumers
accessing the enclosing type from a property wrappers isn't "officially" supported yet

Check points:

Added new tests to cover change, if needed
All unit tests pass
All integration tests pass
Documentation update for the change if required
PR title conforms to conventional commit style

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2021-03-01T23:19:17Z

Codecov Report

Merging #1080 (4977270) into main (3ac0b2c) will increase coverage by 0.54%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1080      +/-   ##
==========================================
+ Coverage   57.98%   58.53%   +0.54%     
==========================================
  Files         519      650     +131     
  Lines       14888    19094    +4206     
==========================================
+ Hits         8633    11176    +2543     
- Misses       6255     7918    +1663

Flag	Coverage Δ
API_plugin_unit_test	`54.53% <ø> (?)`
AWSPluginsCore	`74.47% <ø> (?)`
Amplify	`48.04% <ø> (ø)`
Analytics_plugin_unit_test	`73.41% <ø> (ø)`
Auth_plugin_unit_test	`82.47% <ø> (ø)`
DataStore_plugin_unit_test	`73.06% <ø> (ø)`
Predictions_plugin_unit_test	`33.48% <ø> (ø)`
Storage_plugin_unit_test	`57.72% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../DataStore/Model/Internal/Schema/ModelSchema.swift	`85.29% <0.00%> (-2.95%)`	⬇️
...ginsCore/Auth/Provider/IAMCredentialProvider.swift	`0.00% <0.00%> (ø)`
.../AWSS3StorageService+GetPreSignedURLBehavior.swift	`0.00% <0.00%> (ø)`
...egoryPlugin/Reachability/AmplifyReachability.swift	`0.00% <0.00%> (ø)`
...PICategoryPluginConfiguration+EndpointConfig.swift	`47.12% <0.00%> (ø)`
.../RequestInterceptor/IAMURLRequestInterceptor.swift	`0.00% <0.00%> (ø)`
...ginsCore/Model/Decorator/PaginationDecorator.swift	`87.50% <0.00%> (ø)`
.../Auth/Provider/AmplifyAWSCredentialsProvider.swift	`0.00% <0.00%> (ø)`
...gins/Core/AWSPluginsCore/Auth/AWSAuthService.swift	`35.13% <0.00%> (ø)`
...in/Request/StorageUploadDataRequest+Validate.swift	`100.00% <0.00%> (ø)`
... and 123 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ac0b2c...7bb1d9d. Read the comment docs.

palpatim · 2021-03-03T16:41:10Z

General question: Is there an opportunity to use an existing schema definition such as JSON Schema, Swagger, or Smithy? These are generally for service-side APIs, and I haven't investigated their applicability for a CLI use case, but it would be great if we could re-use prior art rather than creating a new standard. :D

palpatim · 2021-03-03T16:47:32Z

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandEncodable.swift

+    case parameters
+}
+
+protocol CLICommandEncodable: Encodable {


Is there an existing CLICommand that it would make more sense to add Encodable conformance to? If not, "Encodable" in the protocol name seems unnecessary, unless it's the reason for the protocol's existence, as in marker protocols or something where the protocol is tightly bound to the behavior the protocol describes--see discussion at https://swift.org/documentation/api-design-guidelines/#naming

there's no existing CLICommand, the only reason for the CLICommandEncodable's existence is to have Encodable conformance -- but I'm thinking that would make sense to just have a generic CLICommand protocol and let it conform to Encodable. Thoughts?

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandEncodable.swift

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandEncodableParameter.swift

palpatim · 2021-03-03T16:51:25Z

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandEncodableParameter.swift

+import Foundation
+
+/// Encodable representation of CLI parameter.
+enum CLICommandEncodableParameter: Hashable {


As above, would it make more sense to add Encodable performance to an existing CLICommandParameter protocol or concrete type, rather than creating a new type?

Unfortunately there's no existing CLICommandParameter type as the parameters are being handled by the ArgumentParser library via property wrappers (@Option, @Flag) -- the only reason for having an extra type is to retain information about commands' parameters and use it for generating the schema

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandEncodableParameter.swift

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandGenerateJSONSchema.swift

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandEncodable.swift

palpatim · 2021-03-03T16:59:56Z

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandGenerateJSONSchema.swift

+    var commands: [AnyCLICommandEncodable] = []
+
+    init() {
+        for command in AmplifyXcode.configuration.subcommands where command != CLICommandGenerateJSONSchema.self {


Why don't we generating a schema for the "Generate" command?

Updated, generate-schema command generates a schema of itself :)

palpatim · 2021-03-03T17:16:27Z

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandEncodable.swift

+protocol CLICommandEncodable: Encodable {
+    static var commandName: String { get }
+    static var abstract: String { get }
+    static var paramsRegistry: CLICommandEncodableParametersRegistry { get }


Do we need an additional wrapping type here, or can we just make params a Set with direct get access?

We actually don't need it, I just wanted to add a layer of abstraction and hide the underlying storage, Set. (I don't even really like the name :P )

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/Support/ArgumentParser+CLICommandEncodable.swift

Co-authored-by: Tim Schmelter <schmelte@amazon.com>

palpatim

Approved w/comments

palpatim · 2021-03-05T14:45:44Z

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/Support/ArgumentParser+CLICommand.swift

 extension Option where Value: ExpressibleByArgument {
-    init(wrappedValue: Value, name: String, help: String, _ registry: CLICommandEncodableParametersRegistry) {
+    init(wrappedValue: Value, name: String, help: String, _ parameters: inout Set<CLICommandParameter>) {


Minor naming suggestion to clarify the intent of the parameters and why it's an inout:

Suggested change

init(wrappedValue: Value, name: String, help: String, _ parameters: inout Set<CLICommandParameter>) {

init(wrappedValue: Value, name: String, help: String, updating parameters: inout Set<CLICommandParameter>) {

Result at the call site would look like:

@Option(name: "path", help: "Project base path", updating: &parameters) ...

good call, thanks!

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandImportConfig.swift

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandImportModels.swift

palpatim · 2021-03-05T14:49:55Z

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/CLICommandParameter.swift

+/// and annotated with `@propertyWrapper`s `@Option`, `@Flag` and `@Argument` provided by `ArgumentParser`.
+/// `ArgumentParser` derives parameters names from property names (i.e.,  an`outputPath` option becomes `--output-path`)
+/// making thus impossible to reliably generate a JSON representation of a command and its parameters.
+/// Therefore we use the following enum to keep track of each parameter and their attributes (name, type and help text).


palpatim · 2021-03-05T14:50:32Z

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/Support/ArgumentParser+CLICommand.swift

    }

-    init(name: String, help: String, _ registry: CLICommandEncodableParametersRegistry) {
+    init(name: String, help: String, _ parameters: inout Set<CLICommandParameter>) {


Suggested change

init(name: String, help: String, _ parameters: inout Set<CLICommandParameter>) {

init(name: String, help: String, updating parameters: inout Set<CLICommandParameter>) {

palpatim · 2021-03-05T14:50:50Z

AmplifyTools/AmplifyXcode/Sources/AmplifyXcode/Support/ArgumentParser+CLICommand.swift

    }
 }

 extension Flag where Value == Bool {
-    init(wrappedValue: Value, name: String, help: String, _ registry: CLICommandEncodableParametersRegistry) {
+    init(wrappedValue: Value, name: String, help: String, _ parameters: inout Set<CLICommandParameter>) {


Suggested change

init(wrappedValue: Value, name: String, help: String, _ parameters: inout Set<CLICommandParameter>) {

init(wrappedValue: Value, name: String, help: String, updating parameters: inout Set<CLICommandParameter>) {

amplify-xcode: generate JSON schema

d82d988

diegocstn marked this pull request as ready for review February 26, 2021 20:50

diegocstn mentioned this pull request Feb 27, 2021

Add URL query parameter encoding to AWSSignature aws-amplify/aws-sdk-ios#3378

Closed

Delete old CLICommandGenerateBindings.swift

8f5791a

feat: fix typo in command name

9e19aa0

diegocstn requested a review from a team March 2, 2021 16:40

palpatim reviewed Mar 3, 2021

View reviewed changes

diegocstn and others added 7 commits March 3, 2021 10:32

feat(amplify-xcode): apply suggestions from code review (spacing nit)

d252db9

Co-authored-by: Tim Schmelter <schmelte@amazon.com>

feat(amplify-xcode): address PR comments

7996603

feat(amplify-xcode): add docs, rename CLICommandEncodable

53b800c

feat(amplify-xcode): re-generate xcodeproj file

6803e34

feat(amplify-xcode): re-run swiftformat

b64ebad

feat(amplify-xcode): generate-schema generates schema of itself

f0273a7

feat(amplify-xcode): add docs around schema generations

1cf4e2d

palpatim approved these changes Mar 5, 2021

View reviewed changes

diegocstn added 2 commits March 5, 2021 13:49

feat(amplify-xcode): add param label to clarify usage of parameters ref

4977270

feat(amplify-xcode): address PR comments

7bb1d9d

diegocstn merged commit 80861a0 into main Mar 5, 2021

diegocstn deleted the feat/amplify-xcode-bindings branch March 5, 2021 22:27

mojodna mentioned this pull request Jul 6, 2021

[AWSSignatureV4Signer generateQueryStringForSignatureV4WithCredentialProvider: ...] doesn't handle spaces (or other characters requiring encoding) correctly aws-amplify/aws-sdk-ios#3215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(amplify-xcode): generate JSON schema #1080

feat(amplify-xcode): generate JSON schema #1080

diegocstn commented Feb 26, 2021 •

edited

codecov bot commented Mar 1, 2021 •

edited

palpatim commented Mar 3, 2021

palpatim Mar 3, 2021

diegocstn Mar 3, 2021

palpatim Mar 3, 2021

diegocstn Mar 3, 2021

palpatim Mar 3, 2021

brennanMKE Mar 4, 2021

diegocstn Mar 4, 2021

palpatim Mar 3, 2021

diegocstn Mar 3, 2021

palpatim left a comment

palpatim Mar 5, 2021

diegocstn Mar 5, 2021

palpatim Mar 5, 2021

palpatim Mar 5, 2021

palpatim Mar 5, 2021

	init(wrappedValue: Value, name: String, help: String, _ parameters: inout Set<CLICommandParameter>) {
	init(wrappedValue: Value, name: String, help: String, updating parameters: inout Set<CLICommandParameter>) {

	init(name: String, help: String, _ parameters: inout Set<CLICommandParameter>) {
	init(name: String, help: String, updating parameters: inout Set<CLICommandParameter>) {

feat(amplify-xcode): generate JSON schema #1080

feat(amplify-xcode): generate JSON schema #1080

Conversation

diegocstn commented Feb 26, 2021 • edited

codecov bot commented Mar 1, 2021 • edited

Codecov Report

palpatim commented Mar 3, 2021

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

Choose a reason for hiding this comment

palpatim 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

diegocstn commented Feb 26, 2021 •

edited

codecov bot commented Mar 1, 2021 •

edited