-
Notifications
You must be signed in to change notification settings - Fork 11
pubsub: Enable implicit GetQueueUrl on initialization #183
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
============================================
+ Coverage 83.59% 83.61% +0.02%
Complexity 90 90
============================================
Files 150 150
Lines 8038 8051 +13
Branches 941 940 -1
============================================
+ Hits 6719 6732 +13
Misses 875 875
Partials 444 444
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| throw new InvalidArgumentException("Subscription name cannot be null or empty"); | ||
| } | ||
| if (!subscriptionName.startsWith("https://sqs.") || !subscriptionName.contains(".amazonaws.com/")) { | ||
| if (subscriptionName.startsWith("https://")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a random check, why would we check for https prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current logic only accepts queue names, so I'm using https:// to distinguish between URLs and names.
Or we could support both input formats:
- If the consumer passes a queue name, we call GetQueueUrl implicitly.
- If the consumer passes a full queue URL, it's a bit redundant for us to still call GetQueueUrl during client initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean why do we check only for https://, there are can n number of bad inputs, such as clients can just pass the url without https:// prefix. We can just rely on getting the exception from the AWS that the resource doesn't exist in case of bad input or maybe a regex for a valid queue name if we really need pre-validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks I understand, I removed the prefix check and let AWS handle invalid cases
| } | ||
|
|
||
| // get the full queue URL from the queue name | ||
| this.topicName = getQueueUrl(this.topicName, sqsClient); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call it topicUrl now, we shouldn't replace the existing topicName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, we shouldn't overwrite the original topicName. I have renamed it to topicUrl to keep both values clear
| Builder withSubscriptionUrl(String subscriptionUrl) { | ||
| this.subscriptionUrl = subscriptionUrl; | ||
| return this; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this method because in integration tests, both topic and subscription may share the same queue. If each build() call triggers GetQueueUrl, WireMock records multiple mappings, which can cause replay mismatches.
By calling GetQueueUrl once and passing the cached URL via withSubscriptionUrl, we can avoid redundant API calls and ensure only one mapping is generated per queue.
Summary
This PR changes the AWS PubSub client to only accept queue names.
If a queue name is provided, we call GetQueueUrl during initialization and store the resolved full URL internally.
Some conventions to follow
docstore:for document store module,blobstorefor Blob Store moduletest:perf: