smithy-lang / smithy-rs

Code generation for the AWS SDK for Rust, as well as server and generic smithy client generation.
Apache License 2.0
500 stars 188 forks source link

Server deserialize operation with required member expects enum Option #1159

Closed guymguym closed 2 years ago

guymguym commented 2 years ago

With #1148 codegen server of S3 model from aws/sdk/aws-models/s3.json gives a bunch of errors like the below :

error[E0308]: `?` operator has incompatible types
    --> src/operation_deser.rs:1238:13
     |
1238 |             crate::http_serde::deser_payload_delete_objects_delete_objects_input_delete(&bytes)?
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `Option`, found struct `Delete`
     |
     = note: `?` operator cannot convert from `Delete` to `Option<Delete>`
     = note: expected enum `Option<Delete>`
              found struct `Delete`

This is the operation and member -

        "com.amazonaws.s3#DeleteObjectsRequest": {
            "type": "structure",
            "members": {
                ...
                "Delete": {
                    "target": "com.amazonaws.s3#Delete",
                    "traits": {
                        "smithy.api#documentation": "<p>Container for the request.</p>",
                        "smithy.api#httpPayload": {},
                        "smithy.api#required": {},
                        "smithy.api#xmlName": "Delete"
                    }
                },
crisidev commented 2 years ago

I guess we need to do another pass at optionality while using the full S3 model as you mentioned in the related issue.

Sorry about the issue and thanks for reporting this!

crisidev commented 2 years ago

All right, I was able to reproduce this issue by using the full S3 model. I'll work on the codegen to understand what did we miss on #1148

crisidev commented 2 years ago

@guymguym when you have some time do you mind to validate this is actually fixed by my change?

guymguym commented 2 years ago

@crisidev I managed to build it without issues too. Thanks. Now I just need to figure out how to work around #1099 since transmute between client and server structs is not an option.

crisidev commented 2 years ago

Apart from the ugliness and intrinsic unsafety (which are already 2 pretty big issues, don't get me wrong), why is transmuting not an option? I have used it for client to server and vice versa.

I am not suggesting you to solve your issue like this, but we don't have a better option right now..

guymguym commented 2 years ago

@crisidev I am not sure, maybe I am doing it wrong? Here is the error:

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
   --> src/s3_server.rs:85:34
    |
85  |                         unsafe { std::mem::transmute::<ServerInput, ClientInput>(i) }
    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
149 |     register_s3_gateway_op2!(PutObjectTagging);
    |     ------------------------------------------ in this macro invocation
    |
    = note: source type: `s3d_smithy_codegen_server_s3::input::PutObjectTaggingInput` (1408 bits)
    = note: target type: `aws_sdk_s3::input::PutObjectTaggingInput` (1472 bits)
    = note: this error originates in the macro `register_s3_gateway_op2` (in Nightly builds, run with -Z macro-backtrace for more info)
crisidev commented 2 years ago

Oook. I think what is happening here is that the type coming from the SDK is "enhanced" with something which is changing its size and mem::transmute is failing for that reason.

I am doing this in a little home project, but I am not using an SDK client, I am just using a vanilla client generated with smithy-rs. That could probably be the culprit..

That said, probably a better option would be to do what is discussed here:

The RFC has been merged, but I honestly never tried this and I don't know if it has already been implemented nor if it really fits your usecase.

guymguym commented 2 years ago

Got it @crisidev but this means I need the structs to be (non-)exhaustive, which is still not the case for the server structs, and will probably never be the case for client structs, right?

guymguym commented 2 years ago

Oh I see that the server structs are already exhaustive... So it's just the client structs that are not.

crisidev commented 2 years ago

Ah right.. So the server structures are now not marked with non-exhaustive (#1148), but the client one will probably never be..

We are in a little bit of a pickle here..

The last option that I have in mind as a short term fix for you is to serialize the "source" struct into a string / bytes and deserialize it into the "destination" structure.

Of course this comes with 2 problems: 1) You are going to allocate memory, possibly a lot :) 2) It will be tricky to do because our types do not implement Serde standard Serialize / Deserialize interface..

guymguym commented 2 years ago

BTW @crisidev I see that for the transmute error I pasted above (PutObjectTaggingInput) the actual reason why transmute refuses to work is that the required fields are now different between these structs - for the client struct all the fields are Option, and for the server only the optional ones do. So I don't think we can hope to use the { ..x } way.

In any case I think I will start looking at generating the conversion asimpl From<ServerStruct> ClientStruct and impl From<ClientStruct> ServerStruct. If you could just give me a quick pointer on where that might fit best in the code, then I can try that.

crisidev commented 2 years ago

Right, makes sense, thanks for digging into this.

The right approach IMO is to work on what you proposed, the problem is that it is not simple as the client codegen and the server codegen are generating different packages, so we would need a third crate, depending on client and server that owns the conversion layer.

I'll have to be honest, I have to think about it.. I'll get back to you.

guymguym commented 2 years ago

@crisidev Maybe we can generate macros - a macro per conversion. The benefit is that it's just a bunch of semi-processed code (valid syntax, but args are not bound to types) and we allow the caller to compile it with actual types.