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

fix: Don't assume that all service bindings existed before #2405

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

ghostbuster91
Copy link
Contributor

Background

  • What do these changes do?

I changed model.expectShape to model.getShape to so that the code doesn't fail if a given service is not present in the old model which can happen in certain cases.

  • Why are they important?

This is a fix for software.amazon.smithy.model.node.ExpectationNotMetException: Shape not found in model: a.b#c that occurs when a new service is being added that references an already existing operation which was also changed in some way.

Full stacktrace:

 at software.amazon.smithy.model.Model.lambda$expectShape$1(Model.java:713)
        at java.base/java.util.Optional.orElseThrow(Optional.java:403)
        at software.amazon.smithy.model.Model.expectShape(Model.java:713)
        at software.amazon.smithy.model.Model.expectShape(Model.java:729)
        at software.amazon.smithy.aws.traits.auth.diff.SigV4Migration.evaluate(SigV4Migration.java:86)
        at software.amazon.smithy.diff.ModelDiff$Builder.lambda$compare$2(ModelDiff.java:293)
        at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:273)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ReduceOps$ReduceTask.doLeaf(ReduceOps.java:960)
        at java.base/java.util.stream.ReduceOps$ReduceTask.doLeaf(ReduceOps.java:934)
        at java.base/java.util.stream.AbstractTask.compute(AbstractTask.java:327)
        at java.base/java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:754)
        at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
        at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
        at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
        at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
        at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)

Testing

  • How did you test these changes?

I minimized the issue and then reproduced it in the unit test.

Links

  • Links to additional context, if necessary
  • Issue #, if applicable (see here for a list of keywords to use for linking issues)

SigV4Migration was introduced in #2245


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…its/auth/diff/SigV4Migration.java

Co-authored-by: Jakub Kozłowski <[email protected]>
Copy link
Contributor

@gosar gosar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks for fixing. I've committed @kubukoz 's suggestion to use isServiceShape. Will wait for CI to run and merge later.

@@ -0,0 +1 @@
[NOTE] ns.foo#NewService: Added service `ns.foo#NewService` | AddedShape
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add newline

…s/traits/diffs/no-old-sigv4-new-service.events
@gosar gosar merged commit 866dae7 into smithy-lang:main Sep 26, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants