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 subtle break of endpoint prefixes due to semver #3318

Merged
merged 13 commits into from
Jan 12, 2024

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Dec 14, 2023

When we released smithy-rs release-2023-12-08, we introduced a silent failure for endpoint prefixes, and not in the newly released version, but in the previous releases. There is a subtle issue with semver that causes this. This PR addresses the endpoint prefix part of this problem. Other PRs will fix other parts that are broken by this semver issue.

The issue is that unstable (0.x) runtime crates are declaring types that get placed into the ConfigBag, and these types are referenced in the ConfigBag across crate boundaries. This by itself isn't a problem, but because our stable 1.x crates depend on the unstable crates, it becomes a problem. By releasing 1.1.0 that depends on 0.61, consumers of 1.x pull in both 0.60 and 0.61. The generated code pulls in 0.60, and the 1.1.x crates pull in 0.61. This is fine since two semver-incompatible crate versions can be in the dependency closure. Thus, the generated code which is using 0.60 is placing a 0.60 type in the ConfigBag, and the runtime crates that pull the type out of the ConfigBag are expecting a 0.61 version of it. This leads to the type not existing upon lookup, which then causes the silent break.

This PR fixes this by moving the EndpointPrefix type and its associated methods into aws-smithy-runtime-api, a stable crate. The aws-smithy-http unstable crate is updated to point to the new stable version so that a patch release of that crate will solve the issue for old versions going forward as well.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@jdisanti
Copy link
Collaborator Author

I think cargo-semver-checks is wrong about this being breaking.

@smithy-lang smithy-lang deleted a comment from github-actions bot Dec 14, 2023
@jdisanti jdisanti marked this pull request as ready for review December 14, 2023 01:49
@jdisanti jdisanti requested review from a team as code owners December 14, 2023 01:49
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

github-merge-queue bot pushed a commit that referenced this pull request Dec 20, 2023
## Motivation and Context
As we discovered in #3318, it's possible to cause unexpected breakage by
releasing runtime crates.
 
This tool automates setting up `aws-sdk-rust` (and manually, you can
copy the patches into `~/.cargo/config.toml` for your entire system), to
simulate the release of new runtime crates.

<img width="1518" alt="Screenshot 2023-12-15 at 12 58 41 PM"
src="https://github.com/smithy-lang/smithy-rs/assets/492903/59d6cb5c-d39c-4e42-98e2-6858d0884449">


### Testing
With this tool (and a small hack—I had to simulate releasing 1.1.100 so
that the versions became the latest), I correctly caught the breaking
changes from the previous release.


----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti enabled auto-merge January 12, 2024 19:11
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti added this pull request to the merge queue Jan 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jan 12, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 12, 2024
This issue addresses a semver compatibility problem similar to the one
described in #3318, except for the
`aws_smithy_http::operation::Metadata` type.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
@jdisanti jdisanti enabled auto-merge January 12, 2024 19:38
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti added this pull request to the merge queue Jan 12, 2024
Merged via the queue into main with commit 30a801a Jan 12, 2024
40 of 41 checks passed
@jdisanti jdisanti deleted the jdisanti-silentbreak-fix-endpoint-prefix branch January 12, 2024 20:28
rcoh pushed a commit that referenced this pull request Jan 12, 2024
This issue addresses a semver compatibility problem similar to the one
described in #3318, except for the
`aws_smithy_http::operation::Metadata` type.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
rcoh pushed a commit that referenced this pull request Jan 12, 2024
When we released smithy-rs release-2023-12-08, we introduced a silent
failure for endpoint prefixes, and not in the newly released version,
but in the previous releases. There is a subtle issue with semver that
causes this. This PR addresses the endpoint prefix part of this problem.
Other PRs will fix other parts that are broken by this semver issue.

The issue is that unstable (0.x) runtime crates are declaring types that
get placed into the `ConfigBag`, and these types are referenced in the
`ConfigBag` across crate boundaries. This by itself isn't a problem, but
because our stable 1.x crates depend on the unstable crates, it becomes
a problem. By releasing 1.1.0 that depends on 0.61, consumers of 1.x
pull in both 0.60 and 0.61. The generated code pulls in 0.60, and the
1.1.x crates pull in 0.61. This is fine since two semver-incompatible
crate versions can be in the dependency closure. Thus, the generated
code which is using 0.60 is placing a 0.60 type in the `ConfigBag`, and
the runtime crates that pull the type out of the `ConfigBag` are
expecting a 0.61 version of it. This leads to the type not existing upon
lookup, which then causes the silent break.

This PR fixes this by moving the `EndpointPrefix` type and its
associated methods into `aws-smithy-runtime-api`, a stable crate. The
`aws-smithy-http` unstable crate is updated to point to the new stable
version so that a patch release of that crate will solve the issue for
old versions going forward as well.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
github-merge-queue bot pushed a commit that referenced this pull request Jan 23, 2024
This issue addresses a semver compatibility problem similar to the one
described in #3318, except
for the storable types in the aws-http crate. I opted to move all of
aws-http into aws-runtime since there was so little in there.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
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.

4 participants