fix: updated u2c logic to filter out urls with negative priority #4416
Conversation
📝 WalkthroughWalkthroughThe changes modify the logic in the host selection process within the service URL handling code to ensure that only hosts with a positive priority are considered when determining the highest-priority host. Specifically, the reduce function now includes an explicit check for the current host's priority being greater than zero before it can be selected. Additionally, a new integration test was added to verify that hosts with a priority of -1 are correctly ignored in host selection, ensuring that only valid hosts are returned by the service URL resolution logic. Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/webex-core/src/lib/services/service-url.js(1 hunks)packages/@webex/webex-core/test/integration/spec/services/services.js(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
packages/@webex/webex-core/src/lib/services/service-url.js (1)
Learnt from: robstax
PR: #3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in packages/@webex/webex-core/test/unit/spec/webex-core.js, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
packages/@webex/webex-core/test/integration/spec/services/services.js (1)
Learnt from: robstax
PR: #3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in packages/@webex/webex-core/test/unit/spec/webex-core.js, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
🧬 Code Graph Analysis (1)
packages/@webex/webex-core/test/integration/spec/services/services.js (1)
packages/@webex/webex-core/src/lib/services/service-url.js (1)
ServiceUrl(9-123)
🪛 GitHub Actions: johnsoter13 is running Pull Request CI
packages/@webex/webex-core/test/integration/spec/services/services.js
[error] 343-343: ReferenceError: negativePriorityUrl is not defined in test case 'handles case where there is a priority of -1 for a service url' in #getServiceFromUrl()
🔇 Additional comments (2)
packages/@webex/webex-core/src/lib/services/service-url.js (1)
78-80: LGTM! Clean implementation of negative priority filtering.The addition of
current.priority > 0 &&to the reduce condition effectively filters out hosts with zero or negative priority from being selected as the priority host. This aligns perfectly with the PR objective to handle u2c backend updates that mark some service URLs with negative priority (-1).The logic ensures that only hosts with positive priority are considered in the priority selection algorithm, while maintaining the existing priority comparison and homeCluster fallback behavior.
packages/@webex/webex-core/test/integration/spec/services/services.js (1)
324-355: Well-designed test case for negative priority filtering.The test effectively validates the core functionality by:
- Creating a ServiceUrl with both positive (1) and negative (-1) priority hosts
- Verifying that the host with negative priority is correctly ignored
- Confirming that the host with positive priority is selected as the priority URL
This test case provides good coverage for the changes made in the
service-url.jsfile and aligns with the PR objectives.
packages/@webex/webex-core/test/integration/spec/services/services.js
Outdated
Show resolved
Hide resolved
packages/@webex/webex-core/test/integration/spec/services/services.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/webex-core/src/lib/services/service-url.js(1 hunks)packages/@webex/webex-core/test/integration/spec/services/services.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
packages/@webex/webex-core/test/integration/spec/services/services.js (1)
Learnt from: robstax
PR: #3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in packages/@webex/webex-core/test/unit/spec/webex-core.js, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
🪛 GitHub Actions: johnsoter13 is running Pull Request CI
packages/@webex/webex-core/test/integration/spec/services/services.js
[error] 343-343: ReferenceError: negativePriorityUrl is not defined in test case 'handles case where there is a priority of -1 for a service url' in #getServiceFromUrl()
🔇 Additional comments (2)
packages/@webex/webex-core/src/lib/services/service-url.js (1)
78-80: LGTM! Correctly filters out negative priority hosts.The addition of
current.priority > 0check ensures that hosts with negative priority values (like -1) are excluded from selection during the reduce operation. This aligns perfectly with the PR objective to filter out URLs with negative priority from the u2c backend.packages/@webex/webex-core/test/integration/spec/services/services.js (1)
324-355: Test correctly verifies negative priority filtering logic.The test properly validates that hosts with priority -1 are ignored during host selection, ensuring only hosts with positive priority are considered. This effectively tests the logic change made in
service-url.js.
packages/@webex/webex-core/test/integration/spec/services/services.js
Outdated
Show resolved
Hide resolved
packages/@webex/webex-core/test/integration/spec/services/services.js
Outdated
Show resolved
Hide resolved
| @@ -75,7 +75,9 @@ const ServiceUrl = AmpState.extend({ | |||
| return this._generateHostUrl( | |||
| filteredHosts.reduce( | |||
| (previous, current) => | |||
| previous.priority > current.priority || !previous.homeCluster ? current : previous, | |||
| current.priority > 0 && (previous.priority > current.priority || !previous.homeCluster) | |||
There was a problem hiding this comment.
I think we need to do a couple more things
- homeCluster should not override priority decisions (this is a problem with the current implementation)
- the array probably needs to be pre-sorted by priority
As it is right now, -1 urls would be removed, but we could still have a logic error based on the following
A. If homeCluster is set to true for an "invalid" FQDN
B. if the order of the array is incorrect
There was a problem hiding this comment.
I also think we need Colin's input on this re: homeCluster since he added this part
There was a problem hiding this comment.
did we decide we don't have to do these? i thought we could merge this as is?
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
by making the following changes
Change Type
The following scenarios were tested
Manually tested, and tests written
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.