-
Notifications
You must be signed in to change notification settings - Fork 47
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
JSONQGet + JSON.INDEX ADD #40
base: master
Are you sure you want to change the base?
Conversation
…JSON and RediSearch, making a GET with Query
Pull Request Test Coverage Report for Build 97
💛 - Coveralls |
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 am not sure whether to implement the RedisJSON2
methods or not.
But I think the interface ReJSON
and the current Handler must not be updated as the users using RedisJSON
does not have support for new methods in RedisJSON2.
type ReJSON interface { | ||
JSONSet(key, path string, obj interface{}, opts ...rjs.SetOption) (res interface{}, err error) | ||
|
||
JSONSetWithIndex(key, path string, obj interface{}, index string) (res interface{}, err error) | ||
|
||
JSONGet(key, path string, opts ...rjs.GetOption) (res interface{}, err error) | ||
|
||
JSONQGet(key string, params ...string) (res interface{}, err error) | ||
|
||
JSONIndexAdd(key, field, path string) (res interface{}, err error) | ||
|
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.
Instead of adding these methods in ReJSON
interface create a separate interface ReJSON2
and embed ReJSON
interfaces in it along with the new methods. And create a separate handler for the ReJSON2, so that the new module won't affect the original one.
Something like this:
type ReJSON interface { | |
JSONSet(key, path string, obj interface{}, opts ...rjs.SetOption) (res interface{}, err error) | |
JSONSetWithIndex(key, path string, obj interface{}, index string) (res interface{}, err error) | |
JSONGet(key, path string, opts ...rjs.GetOption) (res interface{}, err error) | |
JSONQGet(key string, params ...string) (res interface{}, err error) | |
JSONIndexAdd(key, field, path string) (res interface{}, err error) | |
type ReJSON2 interface { | |
ReJSON | |
JSONSetWithIndex(key, path string, obj interface{}, index string) (res interface{}, err error) | |
JSONQGet(key string, params ...string) (res interface{}, err error) | |
JSONIndexAdd(key, field, path string) (res interface{}, err error) | |
} |
func (r *Handler) JSONSetWithIndex(key string, path string, obj interface{}, index string) ( | ||
res interface{}, err error) { | ||
|
||
if r.clientName == rjs.ClientInactive { | ||
return nil, rjs.ErrNoClientSet | ||
} | ||
return r.implementation.JSONSetWithIndex(key, path, obj, index) | ||
} | ||
|
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.
Create a separate handler for the ReJSON2, and implement the ReJSON2
interface on it.
You can also wrap the original handler and create a new handler for ReJSON2.
func (r *Handler) JSONQGet(key string, params ...string) (res interface{}, err error) { | ||
if r.clientName == rjs.ClientInactive { | ||
return nil, rjs.ErrNoClientSet | ||
} | ||
return r.implementation.JSONQGet(key, params...) | ||
} | ||
|
||
// JSONAddIndex used to get a json object | ||
// | ||
// ReJSON syntax: | ||
// JSON.INDEX ADD <index> <field> <path> | ||
// | ||
func (r *Handler) JSONIndexAdd(index, field, path string) (res interface{}, err error) { | ||
if r.clientName == rjs.ClientInactive { | ||
return nil, rjs.ErrNoClientSet | ||
} | ||
return r.implementation.JSONIndexAdd(index, field, path) | ||
} |
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.
Same for all new methods
ReJSONCommandQGET ReJSONCommandID = 20 | ||
ReJSONCommandINDEXADD ReJSONCommandID = 21 | ||
ReJSONCommandSETINDEX ReJSONCommandID = 22 |
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.
Just a suggestion to differentiate from others
ReJSONCommandQGET ReJSONCommandID = 20 | |
ReJSONCommandINDEXADD ReJSONCommandID = 21 | |
ReJSONCommandSETINDEX ReJSONCommandID = 22 | |
ReJSON2CommandQGET ReJSONCommandID = 20 | |
ReJSON2CommandINDEXADD ReJSONCommandID = 21 | |
ReJSON2CommandSETINDEX ReJSONCommandID = 22 |
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.
Also, update the test cases to achieve significant code coverage.
Hey @breno12321 👋 |
Created functions to use the RedisJSON2 JSON.QGet, JSON.INDEX add and a JSON.SETWithIndex