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

Non-required streaming blob shape members are codegenned without Option #1302

Open
david-perez opened this issue Apr 5, 2022 · 3 comments
Open
Labels
blocked breaking-change This will require a breaking change

Comments

@david-perez
Copy link
Contributor

This model:

$version: "1.0"

namespace com.amazonaws.simple

use aws.protocols#restJson1

@restJson1
@title("SimpleService")
service SimpleService {
    operations: [
        AnOperation,
    ],
}

@http(uri: "/operation", method: "GET")
operation AnOperation {
    input: AnOperationInput,
    output: AnOperationOutput,
}

structure AnOperationInput {
    @httpPayload
    blob: StreamingBlob
}

structure AnOperationOutput {
}

@streaming
blob StreamingBlob

Makes both client and server yield in input.rs:

#[allow(missing_docs)] // documentation missing in model
#[non_exhaustive]
pub struct AnOperationInput {
    #[allow(missing_docs)] // documentation missing in model
    pub blob: aws_smithy_http::byte_stream::ByteStream,
}

Note that the member shape is not marked @required, yet the field blob is not optional.


The issue lies in StreamingTraitSymbolProvider, that directly returns a ByteStream if it detects the shape is streaming, without taking into account that the base symbol provider(s) might return an Option<Blob> instead of a Blob.

https://github.com/awslabs/smithy-rs/blob/e78d40f0d52651c16efb671c749d852d61b10565/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/StreamingTraitSymbolProvider.kt#L42-L47

I'm currently trying to make a fix along these lines (note that it's only looking in references one level deep):

diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/StreamingTraitSymbolProvider.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/StreamingTraitSymbolProvider.kt
index af7dfa60..72e8babf 100644
--- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/StreamingTraitSymbolProvider.kt
+++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/StreamingTraitSymbolProvider.kt
@@ -6,6 +6,7 @@
 package software.amazon.smithy.rust.codegen.smithy
 
 import software.amazon.smithy.codegen.core.Symbol
+import software.amazon.smithy.codegen.core.SymbolReference
 import software.amazon.smithy.model.Model
 import software.amazon.smithy.model.shapes.BlobShape
 import software.amazon.smithy.model.shapes.MemberShape
@@ -27,23 +28,40 @@ class StreamingShapeSymbolProvider(private val base: RustSymbolProvider, private
     WrappingSymbolProvider(base) {
     override fun toSymbol(shape: Shape): Symbol {
         val initial = base.toSymbol(shape)
-        // We are only targetting member shapes
+        // We are only targeting member shapes.
         if (shape !is MemberShape) {
             return initial
         }
         val target = model.expectShape(shape.target)
         val container = model.expectShape(shape.container)
 
-        // We are only targeting output shapes
+        // We are only targeting operation input and output shapes.
         if (!(container.hasTrait<SyntheticOutputTrait>() || container.hasTrait<SyntheticInputTrait>())) {
             return initial
         }
 
-        // We are only targeting streaming blobs
+        // We are only targeting streaming blobs.
         return if (target is BlobShape && shape.isStreaming(model)) {
             RuntimeType.byteStream(config().runtimeConfig).toSymbol().toBuilder().setDefault(Default.RustDefault).build()
+//            val blobSymbol = RuntimeType.Blob(config().runtimeConfig).toSymbol()
+//            if (initial == blobSymbol) {
+//                RuntimeType.byteStream(config().runtimeConfig).toSymbol().toBuilder().setDefault(Default.RustDefault).build()
+//            } else {
+//                val renamedReferences = initial.references.map {
+//                    if (it.symbol == blobSymbol) {
+//                        it.toBuilder().symbol(
+//                            RuntimeType.byteStream(config().runtimeConfig).toSymbol()
+//                                .toBuilder().setDefault(Default.RustDefault)
+//                                .build()
+//                        ).build()
+//                    } else {
+//                        it
+//                    }
+//                }
+//                initial.toBuilder().references(renamedReferences).build()
+//            }
         } else {
-            base.toSymbol(shape)
+            initial
         }
     }
 }
@david-perez
Copy link
Contributor Author

Consultation with Smithy spec team: smithy-lang/smithy#1179

@Velfi Velfi added the blocked label Apr 11, 2022
@jdisanti
Copy link
Collaborator

I think we need resolution on this before SDK GA since it could be a breaking change.

@jdisanti jdisanti added the breaking-change This will require a breaking change label Sep 30, 2022
@jdisanti jdisanti added this to the SDK GA milestone Sep 30, 2022
@david-perez
Copy link
Contributor Author

Note that the spec in IDL v2 was updated so that a streaming member MUST be annotated with required or default. So this is now technically only a violation:

  • in our implementation of the IDL v1 spec; and
  • in our implementation of the IDL v2 spec only when the streaming member also has @clientOptional or part of an @input structure (see structure member optionality).

@jdisanti jdisanti removed the sdk-ga label Apr 4, 2024
@jdisanti jdisanti removed this from the SDK GA milestone Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked breaking-change This will require a breaking change
Projects
None yet
Development

No branches or pull requests

3 participants