Skip to content

MOD-8080, MOD-8055: Fix crash on error handling, Add OSS ACL validation for FT.SUG* commands#5184

Merged
raz-mon merged 7 commits intomasterfrom
razmon-acl_validate_sug_commands
Nov 11, 2024
Merged

MOD-8080, MOD-8055: Fix crash on error handling, Add OSS ACL validation for FT.SUG* commands#5184
raz-mon merged 7 commits intomasterfrom
razmon-acl_validate_sug_commands

Conversation

@raz-mon
Copy link
Collaborator

@raz-mon raz-mon commented Nov 8, 2024

Describe the changes in the pull request

This PR does the following:

  1. Fix a bug we had in which we did not handle a possible error in redis's side upon calling the RedisModule_Call() API function causing a NULL response that we did not take into account.
  2. Adds the key indexes to the command registration of the FT.SUG* commands so that Redis performs the ACL key validations properly.
  3. Removes the extra redirection done by the coordinator on OSS, since we now handle the command in the correct shard with the relevant key.
  4. Tests that we correctly validate user ACL permissions in our FT.SUG* commands.

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@codecov
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.50%. Comparing base (a93aa64) to head (c6e2630).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5184      +/-   ##
==========================================
+ Coverage   86.43%   86.50%   +0.06%     
==========================================
  Files         192      192              
  Lines       34784    34682     -102     
==========================================
- Hits        30066    30001      -65     
+ Misses       4718     4681      -37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@raz-mon raz-mon requested review from GuyAv46 and removed request for kei-nan November 9, 2024 13:55
@raz-mon raz-mon removed the request for review from MeirShpilraien November 9, 2024 14:09
@oshadmi
Copy link
Collaborator

oshadmi commented Nov 10, 2024

@raz-mon Where is this change? (BTW it could be considered a breaking change)

Fixes the key-position flags of the registration of the FT.SUG* commands

@raz-mon
Copy link
Collaborator Author

raz-mon commented Nov 10, 2024

@oshadmi Comment updated, I removed this from this PR.
I do think we should consider adding it, but it is not necessary for this PR.

GuyAv46
GuyAv46 previously approved these changes Nov 10, 2024
@raz-mon raz-mon added this pull request to the merge queue Nov 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 10, 2024
@raz-mon raz-mon added this pull request to the merge queue Nov 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 10, 2024
@raz-mon raz-mon added this pull request to the merge queue Nov 11, 2024
@raz-mon raz-mon removed this pull request from the merge queue due to a manual request Nov 11, 2024
@raz-mon raz-mon requested review from GuyAv46 and oshadmi November 11, 2024 10:20
@raz-mon raz-mon added this pull request to the merge queue Nov 11, 2024
Merged via the queue into master with commit e7eb9d0 Nov 11, 2024
@raz-mon raz-mon deleted the razmon-acl_validate_sug_commands branch November 11, 2024 12:50
redisearch-backport-pull-request bot pushed a commit that referenced this pull request Nov 11, 2024
…on for FT.SUG* commands (#5184)

* WIP add test

* wip

* Fix error-handling crash, add OSS support for ACL validation for the FT.SUG* commands

* Revert command registration change (for now..)

* Fix test

* Remove redirection for FT.SUG* in OSS

* Fix tests

(cherry picked from commit e7eb9d0)
@redisearch-backport-pull-request
Copy link
Contributor

github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024
…lidation for FT.SUG* commands (#5196)

MOD-8080, MOD-8055: Fix crash on error handling, Add OSS ACL validation for FT.SUG* commands (#5184)

* WIP add test

* wip

* Fix error-handling crash, add OSS support for ACL validation for the FT.SUG* commands

* Revert command registration change (for now..)

* Fix test

* Remove redirection for FT.SUG* in OSS

* Fix tests

(cherry picked from commit e7eb9d0)

Co-authored-by: Raz Monsonego <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants