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

metadata validation is stricter #1352

Closed
daddykotex opened this issue Aug 17, 2022 · 2 comments · Fixed by #1521
Closed

metadata validation is stricter #1352

daddykotex opened this issue Aug 17, 2022 · 2 comments · Fixed by #1521
Labels
documentation This is a problem with documentation.

Comments

@daddykotex
Copy link
Contributor

Previously, we've had code like that:

metadata thing = [
  {
    value: ActorId,
  },
  {
    value: examples.movies#ActorId,
  }
]

namespace examples.movies

string ActorId
string MovieId

And it was fine. Both the short version (no namespace), and the fully qualified name resolved correctly. When updating to 1.23.0 (at least in my language-server), I see the following:

Screen Shot 2022-08-17 at 09 19 31

The specific error I see is: SyntacticShapeIdTarget: Syntactic shape ID ActorIddoes not resolve to a valid shape ID:smithy.api#ActorId. Did you mean to quote this string? Are you missing a model file?

Although this makes sense to me, I'm surprised that this shows up. Is this expected?

I think it's coming from: a93711d

In all cases, I'm not asking for a fix (using the fully qualified name works), maybe just some explanation. This can serve as documentation for people who see this error.

@mtdowling
Copy link
Member

The metadata section is defined outside of any namespace, so the parser has to assume smithy.api, the prelude namespace, if a syntactic shape ID is used. The previous behavior was incorrect if this resolved to examples.movies#ActorId previously. There's an example of the expected behavior in the Object Keys section here from the v1 spec: https://awslabs.github.io/smithy/1.0/spec/core/idl.html#syntactic-shape-ids (and also present in the v2 spec).

We'll take an action item to update the spec to make this even more clear though.

@daddykotex
Copy link
Contributor Author

That was the result of our internal discussion, so I'm not surprised. Thanks for confirming.

Feel free to close when the spec has been updated or right away if you want.

@mtdowling mtdowling added the documentation This is a problem with documentation. label Sep 5, 2022
mtdowling added a commit that referenced this issue Dec 1, 2022
mtdowling added a commit that referenced this issue Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants