fix: resolve compatibility test failures#4
Merged
Conversation
Contributor
Reviewer's GuideAdjusts multiple AWS service emulations and compatibility tests to match current botocore/boto3 expectations, including S3Tables REST-JSON routing and ARN handling, Route53 XML response shapes and request parameters, Scheduler REST-JSON behavior, ServerlessRepo casing/parameter compatibility, Bedrock/Textract/CloudWatch/EventBridge behavior tweaks, and test harness improvements (dynamic ports, startup diagnostics) so that all compatibility tests pass. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
tests
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_start_server_error, the error message claims the server did not start within 30s while_wait_for_serveris called withtimeout=5; consider wiring the actual timeout value into the message to avoid confusion during debugging. - The
try/except TypeErrorfallbacks intest_traffic_policy_instancemake it harder to notice unexpectedTypeErrors from other sources; checking the botocore version or operation model up front and selecting a single call signature would be more robust. - In the Route 53 CIDR collection and reusable delegation set tests, you removed assertions on
Name/Statewithout replacing them; consider asserting on other stable fields (e.g., IDs or collection contents) so the tests still validate the created resources' shape rather than just that the calls succeed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_start_server_error`, the error message claims the server did not start within 30s while `_wait_for_server` is called with `timeout=5`; consider wiring the actual timeout value into the message to avoid confusion during debugging.
- The `try/except TypeError` fallbacks in `test_traffic_policy_instance` make it harder to notice unexpected `TypeError`s from other sources; checking the botocore version or operation model up front and selecting a single call signature would be more robust.
- In the Route 53 CIDR collection and reusable delegation set tests, you removed assertions on `Name`/`State` without replacing them; consider asserting on other stable fields (e.g., IDs or collection contents) so the tests still validate the created resources' shape rather than just that the calls succeed.
## Individual Comments
### Comment 1
<location path="internal/services/serverlessrepo/provider.go" line_range="511-514" />
<code_context>
return shared.JSONError("BadRequestException", "ApplicationId is required", http.StatusBadRequest), nil
}
- principal, _ := params["OrganizationId"].(string)
+ principal := strParam(params, "OrganizationId", "organizationId")
if principal == "" {
principal, _ = params["Principal"].(string)
}
</code_context>
<issue_to_address>
**suggestion:** Use the same helper when falling back to the Principal parameter for consistency and camelCase support.
The fallback still uses a direct type assertion for `"Principal"`, so a REST-style `"principal"` value would be ignored. For consistency and camelCase support, use `strParam(params, "Principal", "principal")` in the fallback as well.
```suggestion
principal := strParam(params, "OrganizationId", "organizationId")
if principal == "" {
principal = strParam(params, "Principal", "principal")
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The conftest unconditionally passed `-config devcloud.yaml`, but the file doesn't exist in the repo. When `-config` is explicit, main.go uses config.Load() which calls os.Exit(1) on missing file. Since stderr was DEVNULL, the error was invisible and _wait_for_server timed out at 30s. - Only pass `-config devcloud.yaml` when the file exists (fallback to embedded defaults) - Capture stderr and include it in the error message for debugging
Address code review comments: use DEVNULL for the long-running server process to avoid blocking, only spawn a short-lived PIPE subprocess on failure. Extract _build_devcloud_cmd and _start_server_error helpers with full test coverage.
Server-side fixes:
- bedrock: return tags as [{key,value}] list instead of map
- s3tables: strip /v1 prefix in resolveOp for boto3 path routing
- scheduler: disambiguate create/update POST by checking if schedule exists
- scheduler: read GroupName from body params in getSchedule
- serverlessrepo: use strParam for camelCase/PascalCase param compat
- serverlessrepo: emit both cases in CloudFormation response keys
- serverlessrepo: read organizationId in unshareApplication
- textract: convert Tags to [{Key,Value}] list in adapterToMap
Test fixes:
- route53: handle botocore version differences for TPI/CIDR/RDS params
- support: add required issueType/serviceCode/categoryCode params
- textract: use 3-char minimum S3 bucket name
- Wire actual debug timeout value into _start_server_error message - Replace try/except TypeError with botocore model introspection in route53 tests - Add DelegationSetId assertion for reusable delegation set test - Use strParam for Principal fallback in unshareApplication
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Fix S3Tables resolveOp routing (add CreateNamespace at n=3, PutTableEncryption at n=5), fix unit test URL patterns to match actual routing. All 704 compat tests and all Go unit tests pass.
skyoo2003
force-pushed
the
fix/compatibility-server-startup
branch
from
de2c0bb to
40a5697
Compare
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new S3Tables routing logic now duplicates URL parsing in both resolveOp and s3PathParts (including /v1 stripping and ARN collapsing); consider centralizing this into a single helper to reduce the risk of them diverging on future changes.
- In tests/compatibility/conftest.py, _start_server_error reports a 30s startup timeout but the secondary debug wait is hard-coded to 5s; aligning the timeout constants and message would make debugging clearer.
- For the Route53 reusable delegation set and CIDR collection handlers, values like CallerReference and Version are currently hard-coded (e.g., "rds-ref-1", 1); if possible, deriving these from the incoming request would better reflect real API behavior and reduce surprises for clients that inspect them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new S3Tables routing logic now duplicates URL parsing in both resolveOp and s3PathParts (including /v1 stripping and ARN collapsing); consider centralizing this into a single helper to reduce the risk of them diverging on future changes.
- In tests/compatibility/conftest.py, _start_server_error reports a 30s startup timeout but the secondary debug wait is hard-coded to 5s; aligning the timeout constants and message would make debugging clearer.
- For the Route53 reusable delegation set and CIDR collection handlers, values like CallerReference and Version are currently hard-coded (e.g., "rds-ref-1", 1); if possible, deriving these from the incoming request would better reflect real API behavior and reduce surprises for clients that inspect them.
## Individual Comments
### Comment 1
<location path="internal/services/s3tables/provider.go" line_range="800-801" />
<code_context>
+func collapseARN(parts []string, startIdx int) []string {
</code_context>
<issue_to_address>
**issue (bug_risk):** collapseARN now over-collapses and will swallow namespace/table/subresource segments into the ARN
The new implementation advances while the next segment is non-empty and lacks `:`, so for a path like `/buckets/arn:aws:s3tables:...:bucket/my-bucket/namespaces/ns/tables/foo`, it will walk through `my-bucket`, `namespaces`, `ns`, `tables`, and `foo`. `arnEnd` becomes `len(parts)`, so the ARN collapses into the entire tail of the path. Downstream, `resolveOp` and `s3PathParts` no longer see `namespaces`, `tables`, or the table name as separate segments, breaking routing and parameter extraction.
The older version avoided this by stopping at known subresource names. We should restore that behavior (e.g., stop at known subresources or at most join `startIdx` and `startIdx+1`), otherwise any ARN with trailing segments is likely misparsed.
</issue_to_address>
### Comment 2
<location path="internal/services/s3tables/provider.go" line_range="1017-1024" />
<code_context>
-func pathParam(path, key string) string {
+// s3PathParts parses a boto3-style S3Tables URL and returns the extracted
+// bucket ARN (or name), namespace, table name, and subresource.
+func s3PathParts(path string) (bucketARN, namespace, tableName, subresource string) {
+ path = strings.TrimPrefix(path, "/v1")
+ parts := strings.Split(strings.Trim(path, "/"), "/")
+ if len(parts) >= 2 && strings.Contains(parts[1], "arn:") {
+ parts = collapseARN(parts, 1)
+ }
+ // For table URLs, also collapse ARN at position 3
+ if len(parts) >= 4 && parts[0] == "tables" && strings.Contains(parts[3], "arn:") {
+ parts = collapseARN(parts, 3)
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** s3PathParts relies on collapseARN behavior that is no longer correct, compounding the parsing bug for table URLs
Because `s3PathParts` now calls `collapseARN(parts, 1)` (and `collapseARN(parts, 3)` for table URLs), the new `collapseARN` behavior will likely collapse too many segments for paths like `/tables/arn:...:bucket/my-bucket/ns/table/metadata-location`, shifting or emptying `namespace`, `tableName`, and `subresource`.
After fixing `collapseARN`, please ensure we have coverage for:
- `/namespaces/{bucketArn}` and `/namespaces/{bucketArn}/{ns}` → correct `bucketARN` and `namespace`
- `/tables/{bucketArn}/{ns}/{table}` and `/tables/{bucketArn}/{ns}/{table}/{subresource}` → all four return values populated correctly
As written, the interaction between `collapseARN` and `s3PathParts` will cause several callers (e.g. `createTable`, `deleteTable`, `getTableBucketPolicy`) to see incorrect names.
</issue_to_address>
### Comment 3
<location path="internal/services/scheduler/provider.go" line_range="59-65" />
<code_context>
op = resolveOp(req.Method, req.URL.Path)
}
+ // REST-JSON: both create and update use POST to /schedules/{name}.
+ // When resolveOp returns CreateSchedule, check if the schedule already
+ // exists and redirect to UpdateSchedule.
+ if op == "CreateSchedule" {
+ name := scheduleName(req.URL.Path)
+ groupName := scheduleGroup(req.URL.Path)
+ if gp, ok := params["GroupName"].(string); ok && gp != "" {
+ groupName = gp
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** CreateSchedule -> UpdateSchedule dispatch only checks `GroupName` and ignores `groupName` from REST-JSON bodies
The `HandleRequest` shortcut only reads `GroupName` from `params`, but REST-JSON bodies typically use `groupName`, as in your `getSchedule`/`deleteSchedule` handlers. If a client sends only `groupName` in the body, this code will still rely on the path-derived group name (which may be empty/defaulted), leading to incorrect existence checks.
Please update this to resolve both `GroupName` and `groupName` from `params` (e.g. via a shared helper/`strParam`-style function) before calling `GetSchedule`, to stay consistent with the other handlers.
Suggested implementation:
```golang
// REST-JSON: both create and update use POST to /schedules/{name}.
// When resolveOp returns CreateSchedule, check if the schedule already
// exists and redirect to UpdateSchedule.
if op == "CreateSchedule" {
name := scheduleName(req.URL.Path)
groupName := scheduleGroup(req.URL.Path)
// Prefer explicit group name from request parameters if present.
// Support both "GroupName" (camel-case) and "groupName" (lower camel),
// as used by other handlers (e.g. getSchedule/deleteSchedule).
if gp, ok := params["GroupName"].(string); !ok || gp == "" {
if gpLower, okLower := params["groupName"].(string); okLower && gpLower != "" {
gp = gpLower
}
}
if gp != "" {
groupName = gp
}
if _, err := p.store.GetSchedule(name, groupName); err == nil {
op = "UpdateSchedule"
}
}
```
If your codebase already has a shared helper (e.g. a `strParam` function) used by `getSchedule`/`deleteSchedule` to resolve `"GroupName"`/`"groupName"`, you may prefer to:
1. Replace the inline resolution logic here with a call to that helper to keep behavior consistent.
2. Ensure the helper takes the existing path-derived `groupName` as a default/fallback value.
</issue_to_address>
### Comment 4
<location path="internal/services/bedrock/provider_test.go" line_range="187-193" />
<code_context>
rb2 := parseBody(t, resp2)
- tags := rb2["tags"].(map[string]any)
- assert.Equal(t, "test", tags["env"])
+ tagList := rb2["tags"].([]any)
+ tagMap := map[string]string{}
+ for _, t := range tagList {
+ tm := t.(map[string]any)
+ tagMap[tm["key"].(string)] = tm["value"].(string)
+ }
+ assert.Equal(t, "test", tagMap["env"])
// Untag
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for listing tags when no tags are present
Since this test already covers the non-empty `[{"key","value"}]` shape, please also add a case where `ListTagsForResource` is called on a resource with no tags and assert that `tags` is an empty list. That will lock in the expected empty-state behavior for callers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Centralize URL parsing into parseS3TablesPath, replacing fragile collapseARN that could over-collapse segments (Comments 1 & 2) - Fix scheduler CreateSchedule dispatch to check both GroupName and groupName from REST-JSON body (Comment 3) - Add empty-tags assertion to bedrock TestTagging (Comment 4) - Align _start_server_error timeout message with actual value (Overall Comment 2)
Owner
Author
|
@sourcery-ai resolve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_build_devcloud_cmdand_start_server_errorhelpers with full test coverageDEVNULLfor long-running server process to avoid pipe blocking; only spawn PIPE subprocess on failure for stderr capturepyproject.tomlwithpythonpathfor proper module resolutionServer-side fixes
[{key, value}]list instead of map inlistTagsForResource/v1prefix inresolveOpto match boto3's URL pathsGroupNamefrom body params ingetSchedulestrParamhelper for camelCase/PascalCase param compatibility; emit both cases in CloudFormation response keys; readorganizationIdinunshareApplicationTagsto[{Key, Value}]list inadapterToMapTest fixes
issueType/serviceCode/categoryCodeparamsTest plan
go test ./...)