Skip to content

Fix protocol initialization for AwsRestJson1Protocol #1023

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.

Sign up for GitHub

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

Merged
yasmewad merged 4 commits into from
Jan 28, 2026
Merged

Fix protocol initialization for AwsRestJson1Protocol #1023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5ec5c9
Fix protocol initialization for AwsRestJson1Protocol
yasmewad Jan 26, 2026
d044d00
Remove tautological protocol tests per review feedback
yasmewad Jan 27, 2026
3094452
Update aws/server/aws-server-restjson/src/main/java/software/amazon/s…
yasmewad Jan 28, 2026
6ab7a29
Merge branch 'main' into fix-rest-json
sugmanue Jan 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...n/src/main/java/software/amazon/smithy/java/aws/server/restjson/AwsRestJson1Protocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,13 @@ final class AwsRestJson1Protocol extends ServerProtocol {
var httpMethodToMatchers = new HashMap<String, UriMatcherMapBuilder<Operation<?, ?>>>();
for (Service service : services) {
for (var operation : service.getAllOperations()) {
// Only process operations with HTTP trait.
Copy link
Contributor

@adwsingh adwsingh Jan 27, 2026

Choose a reason for hiding this comment

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

With the RestJson protocol, all operations are required to have the HTTP trait, so this situation should not arise in the first place.

On that note, I don’t think this addresses the root cause of the problem, it only treats a symptom.

In my opinion, the correct fix is to make the protocol resolver filter and initialize only the providers that are actually required, based on the protocol traits declared on the service.

Copy link
Contributor Author

@yasmewad yasmewad Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That's a fair callout. I looked into protocol-level filtering first but hit some blockers.

ProtocolResolver gets instantiated with the Server and supports multiple services simultaneously. Consider serviceA with restJson1 and rpcv2Cbor, and serviceB with only rpcv2Cbor. We'd still need handlers for both protocols loaded. Filtering at the constructor level wouldn't work for this case.

I tried implementing filtering anyway and ran into the TraitKey issue. ProtocolTestProtocolProvider provides a ShapeId but no trait class to construct a TraitKey from. Also not sure if protocol traits are even available in the schema at that point since some example services don't define protocols explicitly.

To unblock the issue and open up further discussion, and going through the options with Manuel, I decided to make HTTP_TRAIT retrieval defensive. This lets handlers instantiate even when services don't use restJson1 or have HTTP traits.

My understanding is that the server needs to be aware of all protocol handlers on the classpath to select the appropriate one at runtime. I think changing this pattern could be challenging, though I might be missing something.

In my opinion, the cleanest solution would be adding a filterCandidateServices method to ServerProtocolProvider. Each provider could implement its own filtering logic based on the wire protocol selection spec instead of failing during construction.

What do you think about that approach?

Copy link
Contributor Author

@yasmewad yasmewad Jan 28, 2026

Choose a reason for hiding this comment

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

Merging this and will continue the discussion offline.

var httpTrait = operation.getApiOperation()
.schema()
.expectTrait(TraitKey.HTTP_TRAIT);
.getTrait(TraitKey.HTTP_TRAIT);
if (httpTrait == null) {
continue;
}
String method = httpTrait.getMethod();
String pattern = httpTrait
.getUri()
Expand Down
2 changes: 1 addition & 1 deletion ...in/java/software/amazon/smithy/java/aws/server/restjson/AwsRestJson1ProtocolProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public ShapeId getProtocolId() {
}

@Override
public int priority() {
public int precision() {
return 0;
}
}
2 changes: 1 addition & 1 deletion core/src/main/java/software/amazon/smithy/java/core/schema/Schema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ public final boolean hasTrait(TraitKey<? extends Trait> trait) {
public final <T extends Trait> T expectTrait(TraitKey<T> trait) {
var t = getTrait(trait);
if (t == null) {
throw new NoSuchElementException("Expected trait not found: " + trait.getClass().getName());
throw new NoSuchElementException("Expected trait not found: " + trait.traitClass().getName());
}
return t;
}
Expand Down
2 changes: 1 addition & 1 deletion .../java/software/amazon/smithy/java/protocoltests/harness/ProtocolTestProtocolProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public ShapeId getProtocolId() {
}

@Override
public int priority() {
public int precision() {
return Integer.MAX_VALUE;
}

Expand Down
2 changes: 1 addition & 1 deletion ...r/server-core/src/main/java/software/amazon/smithy/java/server/core/ProtocolResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public final class ProtocolResolver {
public ProtocolResolver(ServiceMatcher serviceMatcher) {
serverProtocolHandlers = SERVER_PROTOCOL_HANDLERS.values()
.stream()
.sorted(Comparator.comparing(ServerProtocolProvider::priority).reversed())
.sorted(Comparator.comparing(ServerProtocolProvider::precision))
.map(p -> p.provideProtocolHandler(serviceMatcher.getAllServices()))
.toList();
this.serviceMatcher = serviceMatcher;
Expand Down
2 changes: 1 addition & 1 deletion ...er-core/src/main/java/software/amazon/smithy/java/server/core/ServerProtocolProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ public interface ServerProtocolProvider {

ShapeId getProtocolId();

int priority();
int precision();
}
4 changes: 2 additions & 2 deletions ...bor/src/main/java/software/amazon/smithy/java/server/rpcv2/RpcV2CborProtocolProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public ShapeId getProtocolId() {
}

@Override
public int priority() {
return 1;
public int precision() {
return 0;
}
}