smithy-lang / smithy

Smithy is a protocol-agnostic interface definition language and set of tools for generating clients, servers, and documentation for any programming language.
https://smithy.io
Apache License 2.0
1.77k stars 208 forks source link

Validation issue with `nestedProperties` and member elision. #2294

Open libre-man opened 4 months ago

libre-man commented 4 months ago

I have a resource like this:

string TenantId

resource TenantResource {
    identifiers: { tenantId: TenantId }
    read: GetTenant
}

And the following models:

@output
structure GetTenantResponse {
    @required
    @nestedProperties
    @httpPayload
    tenant: Tenant
}

structure Tenant for TenantResource {
    $tenantId
}

When compiling this I get: Member tenantId does not target a property or identifier for resource TenantResource.

When I remove the nestedProperties trait I don't get this error anymore.

libre-man commented 4 months ago

If I add @resourceIdentifier("tenantId") to $tenantId it does work.

hpmellema commented 4 months ago

I suspect the root issue here is that the $tenantId field is missing a @required trait. Elided resource Identifiers should always be required fields. The error messages returned for elided identifiers are currently a bit unclear and probably need to be updated.

libre-man commented 4 months ago

Even adding a required trait does not help unfortunately.

natehardison commented 4 months ago

Same issue -- adding @required does not help, but adding @resourceIdentifier does

sugmanue commented 4 months ago

Thomas and Nate

Care to provide a self-contained minimal model we can use to reproduce the issue?

I just tried with this and validates just fine:

$version: "2"

namespace com.example

string TenantId

resource TenantResource {
    identifiers: { tenantId: TenantId }
    read: GetTenant
}

@readonly
operation GetTenant {
    input: GetTenantRequest
    output: GetTenantResponse
}

@input
structure GetTenantRequest for TenantResource {
    @required
    $tenantId
}

@output
structure GetTenantResponse {
    @required
    @nestedProperties
    @httpPayload
    tenant: Tenant
}

structure Tenant for TenantResource {
    $tenantId
}
natehardison commented 4 months ago

Sure, building off of @libre-man's starting point:

$version: "2"

namespace com.example

string TenantId

resource TenantResource {
    identifiers: { tenantId: TenantId }
    properties: { name: String }
    read: GetTenant
}

@readonly
operation GetTenant {
    input := for TenantResource {
        @required
        $tenantId
    }

    output := for TenantResource {
        @required
        @nestedProperties
        data: Tenant
    }
}

structure Tenant for TenantResource {
    $tenantId
    $name
}

example validation fail:

smithy validate example.smithy

──  ERROR  ──────────────────────────────────────── ResourceOperationInputOutput
Shape: com.example#Tenant$tenantId
File:  example.smithy:29:5

28| structure Tenant for TenantResource {
29|     $tenantId
  |     ^

Member tenantId does not target a property or identifier for resource
com.example#TenantResource

FAILURE: Validated 226 shapes (ERROR: 1)

Notably, if I remove the properties field entirely from TenantResource, then things validate just fine.

Am using Smithy version 1.49.0.

medwards commented 4 weeks ago

I don't think this is the same as libre-man's model. If you remove nestedProperties you get

Member data does not target a property or identifier for resource com.example#TenantResource

but "When I remove the nestedProperties trait I don't get this error anymore."

I found this issue because of the "Member data does not target" error. It may not be the same root cause as libre-man but I found closely reading Resource property binding validation to be very helpful in figuring out what was wrong.