Skip to content

Commit

Permalink
Allow 'null' variants in unions (#3481)
Browse files Browse the repository at this point in the history
## Motivation and Context
- awslabs/aws-sdk-rust#1095

## Description
Update the JSON parser generator to allow for `null` to be set in
unions. Servers can send unions like this:
```json
{
  "AmazonElasticsearchParameters": null,
  "AmazonOpenSearchParameters": null,
  "AppFlowParameters": null,
  "AthenaParameters": null,
  "AuroraParameters": null,
  "AuroraPostgreSqlParameters": null,
  "AwsIotAnalyticsParameters": null,
  "BigQueryParameters": null,
  "DatabricksParameters": null,
  "Db2Parameters": null,
  "DenodoParameters": null,
  "DocumentDBParameters": null,
  "DremioParameters": null,
  "ExasolParameters": null,
  "GoogleAnalyticsParameters": null,
  "JiraParameters": null,
  "MariaDbParameters": null,
  "MongoAtlasParameters": null,
  "MongoDBParameters": null,
  "MySqlParameters": null,
  "OracleParameters": null,
  "PostgreSqlParameters": null,
  "PrestoParameters": null,
  "RdsParameters": null,
  "RedshiftParameters": null,
  "S3Parameters": {
    "IsUploaded": false,
    "ManifestFileLocation": {
      "Bucket": "deided-bucket.prod.us-east-1",
      "Key": "sales/manifest.json"
    },
    "RoleArn": null
  },
  "SalesforceParameters": null,
  "SapHanaParameters": null,
  "ServiceNowParameters": null,
  "SnowflakeParameters": null,
  "SparkParameters": null,
  "SqlServerParameters": null,
  "StarburstParameters": null,
  "TeradataParameters": null,
  "TrinoParameters": null,
  "TwitterParameters": null
}
```

This caused our parser to fail.

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
- [x] New unit test
- [x] Dry run against new [smithy protocol
test](smithy-lang/smithy#2180)

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
rcoh authored Mar 15, 2024
1 parent 0b35a09 commit 23cdff1
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 4 deletions.
5 changes: 4 additions & 1 deletion build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,11 @@ buildscript {
}

allprojects {
val allowLocalDeps: String by project
repositories {
// mavenLocal()
if (allowLocalDeps.toBoolean()) {
mavenLocal()
}
mavenCentral()
google()
}
Expand Down
1 change: 0 additions & 1 deletion buildSrc/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ plugins {
repositories {
mavenCentral()
google()
/* mavenLocal() */
}

// Load properties manually to avoid hard coding smithy version
Expand Down
2 changes: 0 additions & 2 deletions codegen-client-test/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,6 @@ val allCodegenTests = listOf(
),
ClientTest("aws.protocoltests.misc#QueryCompatService", "query-compat-test", dependsOn = listOf("aws-json-query-compat.smithy")),
).map(ClientTest::toCodegenTest)
// use this line to run just one test
// .filter { it.module == "query-compat-test" }

project.registerGenerateSmithyBuildTask(rootProject, pluginName, allCodegenTests)
project.registerGenerateCargoWorkspaceTask(rootProject, pluginName, allCodegenTests, workingDirUnderBuildDir)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.withBlockTemplate
import software.amazon.smithy.rust.codegen.core.smithy.CodegenContext
import software.amazon.smithy.rust.codegen.core.smithy.CodegenTarget
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType.Companion.preludeScope
import software.amazon.smithy.rust.codegen.core.smithy.canUseDefault
import software.amazon.smithy.rust.codegen.core.smithy.customize.NamedCustomization
import software.amazon.smithy.rust.codegen.core.smithy.customize.Section
Expand Down Expand Up @@ -121,6 +122,7 @@ class JsonParserGenerator(
"skip_to_end" to smithyJson.resolve("deserialize::token::skip_to_end"),
"Token" to smithyJson.resolve("deserialize::Token"),
"or_empty" to orEmptyJson(),
*preludeScope,
)

/**
Expand Down Expand Up @@ -560,6 +562,15 @@ class JsonParserGenerator(
*codegenScope,
) {
objectKeyLoop(hasMembers = shape.members().isNotEmpty()) {
rustTemplate(
"""
if let #{Some}(#{Ok}(#{Token}::ValueNull { .. })) = tokens.peek() {
let _ = tokens.next().expect("peek returned a token")?;
continue;
}
""",
*codegenScope,
)
rustTemplate(
"""
let key = key.to_unescaped()?;
Expand Down Expand Up @@ -622,6 +633,16 @@ class JsonParserGenerator(
*codegenScope,
)
}
// If we've gotten to the point where the union had a `{ ... }` section, we can't return None
// anymore. If we didn't parse a union at this point, this is an error.
rustTemplate(
"""
if variant.is_none() {
return Err(#{Error}::custom("Union did not contain a valid variant."))
}
""",
*codegenScope,
)
rust("Ok(variant)")
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,26 @@ class JsonParserGeneratorTest {
""",
)

unitTest(
"allow_null_for_variants",
"""
// __type field should be ignored during deserialization
let input = br#"{ "top": { "choice": { "blob": null, "boolean": null, "int": 5, "long": null, "__type": "value-should-be-ignored-anyway" } } }"#;
let output = ${format(operationGenerator)}(input, test_output::OpOutput::builder()).unwrap().build();
use test_model::Choice;
assert_eq!(Choice::Int(5), output.top.unwrap().choice);
""",
)

unitTest(
"all_variants_null",
"""
// __type field should be ignored during deserialization
let input = br#"{ "top": { "choice": { "blob": null, "boolean": null, "int": null, "long": null, "__type": "value-should-be-ignored-anyway" } } }"#;
let _err = ${format(operationGenerator)}(input, test_output::OpOutput::builder()).expect_err("invalid union");
""",
)

unitTest(
"empty_error",
"""
Expand Down
1 change: 1 addition & 0 deletions gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ kotlin.code.style=official
# codegen
smithyGradlePluginVersion=0.9.0
smithyVersion=1.45.0
allowLocalDeps=false

# kotlin
kotlinVersion=1.9.20
Expand Down

0 comments on commit 23cdff1

Please sign in to comment.