disallow DOS set_queued in remote mode #5510
Conversation
WalkthroughThis pull request adds a conditional check in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Repo as MongoDelegatedOperationRepo
Client->>Repo: update_run_state(id, ExecutionRunState.QUEUED, ...)
note right of Repo: Check if is_remote is True
alt is_remote True
Repo-->>Client: Raise PermissionError("Cannot set queued run_state in remote context.")
else is_remote False
Repo->>Repo: Proceed with run state update
Repo-->>Client: Return success response
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unittests/operators/delegated_tests.py (1)
1364-1384: Consider adding test for state transition to queued in remote mode.While the current tests are good, consider adding a test that verifies the permission check when transitioning from another state (like RUNNING or SCHEDULED) to QUEUED in remote mode.
Example test case:
def test_transition_to_queue_remote_service( self, mock_is_remote_service, mock_get_operator, mock_operator_exists, ): mock_is_remote_service.return_value = True db = delegated_operation.MongoDelegatedOperationRepo() dos = DelegatedOperationService(repo=db) # Create and schedule an operation ctx = ExecutionContext() ctx.request_params = {"foo": "bar"} doc = dos.queue_operation( operator="@voxelfiftyone/operator/foo", label=mock_get_operator.return_value.name, delegation_target="test_target", context=ctx.serialize(), ) self.docs_to_delete.append(doc) # Set it to running doc = dos.set_running(doc.id) self.assertEqual(doc.run_state, ExecutionRunState.RUNNING) # Attempt to transition to queued self.assertRaises(PermissionError, dos.set_queued, doc.id)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fiftyone/factory/repos/delegated_operation.py(1 hunks)tests/unittests/operators/delegated_tests.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (3)
fiftyone/factory/repos/delegated_operation.py (1)
334-337: LGTM! The permission check is well-placed and clear.The added check prevents setting queued state in remote context, which aligns with the PR objective. The error message is descriptive and helpful.
tests/unittests/operators/delegated_tests.py (2)
347-347: LGTM! Good test setup validation and readability improvements.The added assertion verifies remote mode setup, and the visual separators enhance code readability.
Also applies to: 1352-1353, 1359-1360
1364-1384: LGTM! Comprehensive test coverage for the new permission check.The new test method effectively verifies that a PermissionError is raised when attempting to set queued state in remote mode. The test setup is clear and follows established patterns.
What changes are proposed in this pull request?
disallow DOS set_queued in remote mode
How is this patch tested? If it is not, please explain why.
added unit test
tested manually in Teams context
Summary by CodeRabbit