Skip to content
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

MOD-5769: open json key with flags #3916

Merged
merged 8 commits into from Dec 21, 2023
Merged

Conversation

DvirDukhan
Copy link
Contributor

@DvirDukhan DvirDukhan commented Oct 4, 2023

Describe the changes in the pull request

This PR changes the handling of JSON keys opening via rlookup loading. It disables all effects when loading a json key.
This PR depended on RedisJSON/RedisJSON#1095 to be merged since it exposes new LLAPI for RedisJSON

Which issues this PR fixes
2. MOD-5769

Main objects this PR modified

  1. rlookup.c

Mark if applicable

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

src/rejson_api.h Outdated
@@ -37,6 +37,7 @@ typedef struct RedisJSONAPI {
/* RedisJSON functions */
RedisJSON (*openKey)(RedisModuleCtx *ctx, RedisModuleString *key_name);
RedisJSON (*openKeyFromStr)(RedisModuleCtx *ctx, const char *path);
RedisJSON (*openKey_withFlags)(RedisModuleCtx *ctx, RedisModuleString *key_name, int flags);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to move downwards to a V5 version

src/rlookup.c Outdated
@@ -670,7 +670,9 @@ static int getKeyCommonJSON(const RLookupKey *kk, RLookupRow *dst, RLookupLoadOp
char *keyPtr = options->dmd ? options->dmd->keyPtr : (char *)options->keyPtr;
if (!*keyobj) {

*keyobj = japi->openKeyFromStr(ctx, keyPtr);
RedisModuleString* keyName = RedisModule_CreateString(ctx, keyPtr, strlen(keyPtr));
*keyobj = japi->openKey_withFlags(ctx, keyName, REDISMODULE_OPEN_KEY_NOEFFECTS);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that this API is available (using japi->ver)
Or in module init - check for V5 API and if not available - set japi to null

@oshadmi oshadmi marked this pull request as ready for review October 22, 2023 21:07
@oshadmi oshadmi changed the title open json key with flags MOD-5769 MOD-5769: open json key with flags Oct 22, 2023
@codecov
Copy link

codecov bot commented Oct 22, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c98fefa) 83.64% compared to head (822d9a3) 83.65%.

Files Patch % Lines
src/rlookup.c 80.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3916   +/-   ##
=======================================
  Coverage   83.64%   83.65%           
=======================================
  Files         191      191           
  Lines       33510    33518    +8     
=======================================
+ Hits        28031    28039    +8     
  Misses       5479     5479           

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

@raz-mon raz-mon self-requested a review December 21, 2023 13:23
@raz-mon raz-mon added this pull request to the merge queue Dec 21, 2023
Merged via the queue into master with commit cd865c2 Dec 21, 2023
11 checks passed
@raz-mon raz-mon deleted the dvirdu_json_openkey_with_flags branch December 21, 2023 18:42
Copy link

Backport failed for 2.8, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.8
git worktree add -d .worktree/backport-3916-to-2.8 origin/2.8
cd .worktree/backport-3916-to-2.8
git switch --create backport-3916-to-2.8
git cherry-pick -x e00a7331f7ad928dacad0f4bf9eb4f73ed2380c4 d9ac4c377975054fc109f5fa9da5f4894fe9c843 b496bfb2f3495822b3b6003852b82eaebac3c51b e6bd23b7a8d15c66834b4783bdb9c8a462ce8c27

oshadmi pushed a commit that referenced this pull request Dec 21, 2023
* wip

* tests wip

* tests pass

* Use json api ver 5

---------

Co-authored-by: Omer Shadmi <omer.shadmi@redis.com>
Co-authored-by: alonre24 <alonreshef24@gmail.com>
Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
(cherry picked from commit cd865c2)
github-merge-queue bot pushed a commit that referenced this pull request Dec 22, 2023
* wip

* tests wip

* tests pass

* Use json api ver 5

---------

Co-authored-by: Omer Shadmi <omer.shadmi@redis.com>
Co-authored-by: alonre24 <alonreshef24@gmail.com>
Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
(cherry picked from commit cd865c2)

Co-authored-by: DvirDukhan <dvir@redis.com>
raz-mon pushed a commit that referenced this pull request Mar 6, 2024
* wip

* tests wip

* tests pass

* Use json api ver 5

---------

Co-authored-by: Omer Shadmi <omer.shadmi@redis.com>
Co-authored-by: alonre24 <alonreshef24@gmail.com>
Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants