fix: remove InvalidChangeBatch customization #1435

lauzadis · 2024-10-07T17:51:32Z

Route53 no longer returns non-standard InvalidChangeBatch errors, they take the standard form:

<?xml version="1.0"?>\n
<ErrorResponse xmlns="https://route53.amazonaws.com/doc/2013-04-01/">
	<Error>
		<Type>Sender</Type>
		<Code>InvalidChangeBatch</Code>
		<Message>[Tried to delete resource record set [name='test.blerg.com.', type='CNAME'] but the values provided do not match the current values, Tried to create resource record set [name='test.blerg.com.', type='CNAME'] but it already exists]</Message>
	</Error>
	<RequestId>ca6b6a9b-f2be-44c9-8d94-f7dfb3448d10</RequestId>
</ErrorResponse>

Issue #

Description of changes

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

github-actions · 2024-10-07T17:59:50Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-10-07T18:39:51Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

0marperez

Suggestion: Mentioning #242 in the PR description for more context

Suggestion: Adding XML syntax highlighting in the PR description

<?xml version="1.0"?>\n
<ErrorResponse
	xmlns="https://route53.amazonaws.com/doc/2013-04-01/">
	<Error>
		<Type>Sender</Type>
		<Code>InvalidChangeBatch</Code>
		<Message>[Tried to delete resource record set [name='test.blerg.com.', type='CNAME'] but the values provided do not match the current values, Tried to create resource record set [name='test.blerg.com.', type='CNAME'] but it already exists]</Message>
	</Error>
	<RequestId>ca6b6a9b-f2be-44c9-8d94-f7dfb3448d10</RequestId>
</ErrorResponse>

0marperez · 2024-10-07T18:41:49Z

.changes/a9deee4f-8f15-472c-906f-492066275178.json

+{
+    "id": "a9deee4f-8f15-472c-906f-492066275178",
+    "type": "bugfix",
+    "description": "Remove Route53 InvalidChangeBatch customization",


Nit: It would be more descriptive to mention that the customization deals with error responses, e.g. Remove Route53 InvalidChangeBatch error response customization

ianbotsf

+1 to Omar's suggestions

Additionally, how easy/hard would it be to create an E2E test which verifies we correctly deserialize this exception from a real service response? Maybe it's hard but it seems to me such a test would've caught this behavior change earlier and could help guard against a future change.

github-actions · 2024-10-08T22:20:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-10-08T22:28:47Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-10-09T01:26:25Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-10-09T01:59:01Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

0marperez

Suggestion: This test might break if Route53 changes their exception message format. I think we could get rid of the exception message assertion, and we would still be verifying that the exception was deserialized correctly.

ianbotsf · 2024-10-10T16:38:17Z

services/route53/e2eTest/src/InvalidChangeBatchTest.kt

+            assertNotNull(exception.message)
+            assertContains(exception.message, "[Tried to delete resource record set [name='test.blerg.com.', type='CNAME'] but it was not found, RRSet with DNS name test.blerg.com. is not permitted in zone this-is-a-test-hosted-zone-for-aws-sdk-kotlin.com., RRSet of type CNAME with DNS name test.blerg.com. is not permitted as it creates a CNAME loop in the zone.]")


Nit: Asserting on specific exception messages which we do not control is brittle and prone to breakage. Simply deserializing the correct exception should be sufficient for our testing. Users should be catching/handling exceptions based on type anyways—not based on message.

sonarqubecloud · 2024-10-11T13:46:17Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-10-11T13:54:00Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-10-11T15:54:49Z

Affected Artifacts

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
acmpca-jvm.jar	1,169,711	1,169,700	11	0.00%
acmpca-jvm.jar closure	8,967,735	8,967,721	14	0.00%
marketplacereporting-jvm.jar closure	7,914,418	7,914,412	6	0.00%
internetmonitor-jvm.jar closure	8,601,406	8,601,400	6	0.00%
appstream-jvm.jar closure	10,580,122	10,580,115	7	0.00%
greengrassv2-jvm.jar closure	9,213,084	9,213,078	6	0.00%
pcaconnectorad-jvm.jar closure	9,227,324	9,227,318	6	0.00%
amplifyuibuilder-jvm.jar closure	9,586,321	9,586,315	6	0.00%
workdocs-jvm.jar closure	9,656,652	9,656,646	6	0.00%
ecs-jvm.jar closure	11,284,834	11,284,844	-10	-0.00%
redshift-jvm.jar closure	13,729,524	13,729,559	-35	-0.00%
ecs-jvm.jar	3,486,810	3,486,823	-13	-0.00%
redshift-jvm.jar	5,931,500	5,931,538	-38	-0.00%
iotfleetwise-jvm.jar closure	10,152,579	10,152,920	-341	-0.00%
route53resolver-jvm.jar closure	10,186,355	10,186,779	-424	-0.00%
kinesisvideowebrtcstorage-jvm.jar	129,961	129,967	-6	-0.00%
codepipeline-jvm.jar closure	10,580,178	10,581,218	-1,040	-0.01%
outposts-jvm.jar closure	9,026,967	9,028,013	-1,046	-0.01%
elasticache-jvm.jar closure	11,571,333	11,572,963	-1,630	-0.01%
iotfleetwise-jvm.jar	2,354,555	2,354,899	-344	-0.01%
memorydb-jvm.jar closure	9,715,083	9,716,760	-1,677	-0.02%
route53resolver-jvm.jar	2,388,331	2,388,758	-427	-0.02%
codepipeline-jvm.jar	2,782,154	2,783,197	-1,043	-0.04%
route53-jvm.jar closure	10,746,301	10,750,409	-4,108	-0.04%
elasticache-jvm.jar	3,773,309	3,774,942	-1,633	-0.04%
outposts-jvm.jar	1,228,943	1,229,992	-1,049	-0.09%
memorydb-jvm.jar	1,917,059	1,918,739	-1,680	-0.09%
route53-jvm.jar	2,948,277	2,952,388	-4,111	-0.14%
deadline-jvm.jar closure	12,056,175	12,087,292	-31,117	-0.26%
ec2-jvm.jar closure	35,577,750	35,739,273	-161,523	-0.45%
ec2-jvm.jar	27,779,726	27,941,252	-161,526	-0.58%
deadline-jvm.jar	4,258,151	4,289,271	-31,120	-0.73%
databasemigrationservice-jvm.jar closure	12,264,965	12,483,769	-218,804	-1.75%
databasemigrationservice-jvm.jar	4,466,941	4,685,748	-218,807	-4.67%
qconnect-jvm.jar closure	9,912,463	10,874,717	-962,254	-8.85%
qconnect-jvm.jar	2,114,439	3,076,696	-962,257	-31.28%
socialmessaging-jvm.jar	(does not exist)	577,751	-577,751	-100.00%
socialmessaging-jvm.jar closure	(does not exist)	8,375,772	-8,375,772	-100.00%

lauzadis added 2 commits October 7, 2024 13:37

Remove InvalidChangeBatch customization

352ef1c

revert test deletion

4cc6946

lauzadis requested a review from a team as a code owner October 7, 2024 17:51

This comment has been minimized.

Sign in to view

Changelog

be60729

0marperez approved these changes Oct 7, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

ianbotsf approved these changes Oct 7, 2024

View reviewed changes

lauzadis added 2 commits October 8, 2024 11:07

error response

9740446

Add E2E test

dadf763

Configure region

b084874

This comment has been minimized.

Sign in to view

Change error message

142eec9

This comment has been minimized.

Sign in to view

Remove println

1f61f00

This comment has been minimized.

Sign in to view

lauzadis requested a review from ianbotsf October 9, 2024 14:59

0marperez approved these changes Oct 10, 2024

View reviewed changes

ianbotsf approved these changes Oct 10, 2024

View reviewed changes

lauzadis added 2 commits October 11, 2024 09:41

Remove assertion on error message contents

acc251c

ktlint

9d04768

lauzadis merged commit 7f5f09b into main Oct 11, 2024
17 checks passed

lauzadis deleted the fix-route53 branch October 11, 2024 16:21

		assertNotNull(exception.message)
		assertContains(exception.message, "[Tried to delete resource record set [name='test.blerg.com.', type='CNAME'] but it was not found, RRSet with DNS name test.blerg.com. is not permitted in zone this-is-a-test-hosted-zone-for-aws-sdk-kotlin.com., RRSet of type CNAME with DNS name test.blerg.com. is not permitted as it creates a CNAME loop in the zone.]")

fix: remove InvalidChangeBatch customization #1435

fix: remove InvalidChangeBatch customization #1435

Uh oh!

Conversation

lauzadis commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue #

Description of changes

Uh oh!

github-actions bot commented Oct 7, 2024

Uh oh!

This comment has been minimized.

github-actions bot commented Oct 7, 2024

Uh oh!

0marperez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0marperez Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

ianbotsf left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 8, 2024

Uh oh!

github-actions bot commented Oct 8, 2024

Uh oh!

This comment has been minimized.

github-actions bot commented Oct 9, 2024

Uh oh!

This comment has been minimized.

github-actions bot commented Oct 9, 2024

Uh oh!

This comment has been minimized.

0marperez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianbotsf Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 11, 2024

Quality Gate passed

Uh oh!

github-actions bot commented Oct 11, 2024

Uh oh!

github-actions bot commented Oct 11, 2024

Affected Artifacts

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lauzadis commented Oct 7, 2024 •

edited

Loading

0marperez left a comment •

edited

Loading

0marperez left a comment •

edited

Loading