Skip to content

Commit dd0c55f

Browse files
oshadmioshadmi
committed
Rename FORMAT args and change defaults
1 parent 89f1f1b commit dd0c55f

File tree

7 files changed

+162
-104
lines changed

7 files changed

+162
-104
lines changed

‎redis_json/src/c_api.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use std::{
1313
os::raw::{c_char, c_void},
1414
};
1515

16-
use crate::formatter::FormatOptions;
16+
use crate::formatter::ReplyFormatOptions;
1717
use crate::key_value::KeyValue;
1818
use json_path::select_value::{SelectValue, SelectValueType};
1919
use json_path::{compile, create};
@@ -133,7 +133,7 @@ pub fn json_api_get_json<M: Manager>(
133133
str: *mut *mut rawmod::RedisModuleString,
134134
) -> c_int {
135135
let json = unsafe { &*(json.cast::<M::V>()) };
136-
let res = KeyValue::<M::V>::serialize_object(json, &FormatOptions::default());
136+
let res = KeyValue::<M::V>::serialize_object(json, &ReplyFormatOptions::default());
137137
create_rmstring(ctx, &res, str)
138138
}
139139

@@ -147,7 +147,7 @@ pub fn json_api_get_json_from_iter<M: Manager>(
147147
if iter.pos >= iter.results.len() {
148148
Status::Err as c_int
149149
} else {
150-
let res = KeyValue::<M::V>::serialize_object(&iter.results, &FormatOptions::default());
150+
let res = KeyValue::<M::V>::serialize_object(&iter.results, &ReplyFormatOptions::default());
151151
create_rmstring(ctx, &res, str);
152152
Status::Ok as c_int
153153
}

‎redis_json/src/commands.rs

+22-13
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,12 @@
55
*/
66

77
use crate::error::Error;
8-
use crate::formatter::FormatOptions;
8+
use crate::formatter::ReplyFormatOptions;
99
use crate::key_value::KeyValue;
1010
use crate::manager::err_msg_json_path_doesnt_exist_with_param;
1111
use crate::manager::err_msg_json_path_doesnt_exist_with_param_or;
1212
use crate::manager::{Manager, ReadHolder, UpdateInfo, WriteHolder};
13-
use crate::redisjson::{Format, Path};
13+
use crate::redisjson::{Format, Path, ReplyFormat};
1414
use json_path::select_value::{SelectValue, SelectValueType};
1515
use redis_module::{Context, RedisValue};
1616
use redis_module::{NextArg, RedisError, RedisResult, RedisString, REDIS_OK};
@@ -99,25 +99,30 @@ fn default_path(ctx: &Context) -> &str {
9999
/// [INDENT indentation-string]
100100
/// [NEWLINE line-break-string]
101101
/// [SPACE space-string]
102+
/// [FORMAT {STRING|EXPAND1|EXPAND}] /* default is STRING */
102103
/// [path ...]
103104
///
104-
/// TODO add support for multi path
105105
pub fn json_get<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -> RedisResult {
106106
let mut args = args.into_iter().skip(1);
107107
let key = args.next_arg()?;
108108

109109
// Set Capacity to 1 assuming the common case has one path
110110
let mut paths: Vec<Path> = Vec::with_capacity(1);
111111

112-
let mut format_options = FormatOptions::new(is_resp3(ctx));
112+
let mut format_options = ReplyFormatOptions::new(is_resp3(ctx), ReplyFormat::STRING);
113113

114114
while let Ok(arg) = args.next_str() {
115115
match arg {
116116
// fast way to consider arg a path by using the max length of all possible subcommands
117117
// See #390 for the comparison of this function with/without this optimization
118118
arg if arg.len() > JSONGET_SUBCOMMANDS_MAXSTRLEN => paths.push(Path::new(arg)),
119119
arg if arg.eq_ignore_ascii_case(CMD_ARG_FORMAT) => {
120-
format_options.format = Format::from_str(args.next_str()?)?;
120+
// Temporary fix until STRINGS is also supported
121+
let next = args.next_str()?;
122+
if next.eq_ignore_ascii_case("STRINGS") {
123+
return Err(RedisError::Str("ERR wrong reply format"));
124+
}
125+
format_options.format = ReplyFormat::from_str(next)?;
121126
}
122127
arg if arg.eq_ignore_ascii_case(CMD_ARG_INDENT) => {
123128
format_options.indent = Some(args.next_str()?)
@@ -596,7 +601,7 @@ pub fn json_mget<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>)
596601
let path = Path::new(path.try_as_str()?);
597602
let keys = &args[1..args.len() - 1];
598603

599-
let format_options = FormatOptions::new(is_resp3(ctx));
604+
let format_options = ReplyFormatOptions::new(is_resp3(ctx), ReplyFormat::STRING);
600605

601606
// Verify that at least one key exists
602607
if keys.is_empty() {
@@ -736,7 +741,7 @@ where
736741

737742
// Convert to RESP2 format return as one JSON array
738743
let values = to_json_value::<Number>(results, Value::Null);
739-
Ok(KeyValue::<M::V>::serialize_object(&values, &FormatOptions::default()).into())
744+
Ok(KeyValue::<M::V>::serialize_object(&values, &ReplyFormatOptions::default()).into())
740745
}
741746
}
742747

@@ -1368,7 +1373,7 @@ pub fn json_arr_len<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString
13681373

13691374
///
13701375
/// JSON.ARRPOP <key>
1371-
/// [FORMAT {STRING|JSON|EXPAND}]
1376+
/// [FORMAT {STRINGS|EXPAND1|EXPAND}] /* default is STRINGS */
13721377
/// [path [index]]
13731378
///
13741379
pub fn json_arr_pop<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -> RedisResult {
@@ -1377,13 +1382,17 @@ pub fn json_arr_pop<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString
13771382
let key = args.next_arg()?;
13781383

13791384
let is_resp3 = is_resp3(ctx);
1380-
let mut format_options = FormatOptions::new(is_resp3);
1385+
let mut format_options = ReplyFormatOptions::new(is_resp3, ReplyFormat::STRINGS);
13811386

13821387
// Only on RESP3 if the first argument is FORMAT, then the second argument is the format
13831388
// but it's optional so it might be the path
13841389
let path = if let Some(arg) = args.next() {
13851390
if is_resp3 && arg.try_as_str()?.eq_ignore_ascii_case("FORMAT") {
1386-
format_options.format = Format::from_str(args.next_str()?)?;
1391+
let next = args.next_str()?;
1392+
if next.eq_ignore_ascii_case("STRING") {
1393+
return Err(RedisError::Str("ERR wrong reply format"));
1394+
}
1395+
format_options.format = ReplyFormat::from_str(next)?;
13871396
args.next()
13881397
} else {
13891398
// if it's not FORMAT, then it's the path
@@ -1412,9 +1421,9 @@ pub fn json_arr_pop<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString
14121421

14131422
let mut redis_key = manager.open_key_write(ctx, key)?;
14141423
if path.is_legacy() {
1415-
if format_options.format != Format::STRING {
1424+
if format_options.format != ReplyFormat::STRINGS {
14161425
return Err(RedisError::Str(
1417-
"Legacy paths are supported only with FORMAT STRING",
1426+
"Legacy paths are supported only with FORMAT STRINGS",
14181427
));
14191428
}
14201429

@@ -1429,7 +1438,7 @@ fn json_arr_pop_impl<M>(
14291438
ctx: &Context,
14301439
path: &str,
14311440
index: i64,
1432-
format_options: &FormatOptions,
1441+
format_options: &ReplyFormatOptions,
14331442
) -> RedisResult
14341443
where
14351444
M: Manager,

‎redis_json/src/formatter.rs

+21-25
Original file line numberDiff line numberDiff line change
@@ -35,36 +35,32 @@ DEALINGS IN THE SOFTWARE.
3535
use serde_json::ser::Formatter;
3636
use std::io;
3737

38-
use crate::redisjson::Format;
38+
pub use crate::redisjson::{Format, ReplyFormat};
3939

40-
pub struct FormatOptions<'a> {
41-
pub format: Format,
40+
pub struct ReplyFormatOptions<'a> {
41+
pub format: ReplyFormat,
4242
pub indent: Option<&'a str>,
4343
pub space: Option<&'a str>,
4444
pub newline: Option<&'a str>,
4545
pub resp3: bool,
4646
}
4747

48-
impl FormatOptions<'_> {
49-
/// Creates a new FormatOptions with the default values according to the RESP version
50-
pub fn new(resp3: bool) -> Self {
48+
impl ReplyFormatOptions<'_> {
49+
/// Creates a new FormatOptions
50+
pub fn new(resp3: bool, format: ReplyFormat) -> Self {
5151
Self {
52-
format: if resp3 {
53-
Format::EXPAND
54-
} else {
55-
Format::STRING
56-
},
52+
format,
5753
indent: None,
5854
space: None,
5955
newline: None,
6056
resp3,
6157
}
6258
}
6359

64-
/// Returns true if the format is RESP3 and the format is not STRING format
65-
/// STRING format is fully backward compatible with RESP2
60+
/// Returns true if the format is RESP3 and the format is not STRING/STRINGS format
61+
/// STRING/STRINGS format (depending on the command) is fully backward compatible with RESP2
6662
pub fn is_resp3_reply(&self) -> bool {
67-
self.resp3 && self.format != Format::STRING
63+
self.resp3 && self.format != ReplyFormat::STRING && self.format != ReplyFormat::STRINGS
6864
}
6965

7066
/// Checks if the JSON formatting options are the default ones with no overrides
@@ -73,11 +69,11 @@ impl FormatOptions<'_> {
7369
}
7470
}
7571

76-
impl Default for FormatOptions<'_> {
72+
impl Default for ReplyFormatOptions<'_> {
7773
/// Creates a new FormatOptions with the default values matching RESP2
7874
fn default() -> Self {
7975
Self {
80-
format: Format::STRING,
76+
format: ReplyFormat::STRING,
8177
indent: None,
8278
space: None,
8379
newline: None,
@@ -95,7 +91,7 @@ pub struct RedisJsonFormatter<'a> {
9591
}
9692

9793
impl<'a> RedisJsonFormatter<'a> {
98-
pub const fn new(format: &'a FormatOptions) -> Self {
94+
pub const fn new(format: &'a ReplyFormatOptions) -> Self {
9995
RedisJsonFormatter {
10096
current_indent: 0,
10197
has_value: false,
@@ -224,8 +220,8 @@ mod tests {
224220
#[test]
225221
#[allow(clippy::cognitive_complexity)]
226222
fn test_default_formatter() {
227-
let mut formatter = RedisJsonFormatter::new(&FormatOptions {
228-
format: Format::JSON,
223+
let mut formatter = RedisJsonFormatter::new(&ReplyFormatOptions {
224+
format: ReplyFormat::STRING,
229225
indent: None,
230226
space: None,
231227
newline: None,
@@ -288,8 +284,8 @@ mod tests {
288284
#[test]
289285
#[allow(clippy::cognitive_complexity)]
290286
fn test_ident_formatter() {
291-
let mut formatter = RedisJsonFormatter::new(&FormatOptions {
292-
format: Format::JSON,
287+
let mut formatter = RedisJsonFormatter::new(&ReplyFormatOptions {
288+
format: ReplyFormat::STRING,
293289
indent: Some("_"),
294290
space: None,
295291
newline: None,
@@ -352,8 +348,8 @@ mod tests {
352348
#[test]
353349
#[allow(clippy::cognitive_complexity)]
354350
fn test_space_formatter() {
355-
let mut formatter = RedisJsonFormatter::new(&FormatOptions {
356-
format: Format::JSON,
351+
let mut formatter = RedisJsonFormatter::new(&ReplyFormatOptions {
352+
format: ReplyFormat::STRING,
357353
indent: None,
358354
space: Some("s"),
359355
newline: None,
@@ -416,8 +412,8 @@ mod tests {
416412
#[test]
417413
#[allow(clippy::cognitive_complexity)]
418414
fn test_new_line_formatter() {
419-
let mut formatter = RedisJsonFormatter::new(&FormatOptions {
420-
format: Format::JSON,
415+
let mut formatter = RedisJsonFormatter::new(&ReplyFormatOptions {
416+
format: ReplyFormat::STRING,
421417
indent: None,
422418
space: None,
423419
newline: Some("n"),

‎redis_json/src/ivalue_manager.rs

-1
Original file line numberDiff line numberDiff line change
@@ -632,7 +632,6 @@ impl<'a> Manager for RedisIValueJsonKeyManager<'a> {
632632
Ok(v)
633633
},
634634
),
635-
Format::EXPAND => Err("Unsupported format".into()),
636635
}
637636
}
638637

‎redis_json/src/key_value.rs

+25-16
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,12 @@ use serde_json::Value;
1212
use crate::{
1313
commands::{FoundIndex, ObjectLen, Values},
1414
error::Error,
15-
formatter::{FormatOptions, RedisJsonFormatter},
15+
formatter::{RedisJsonFormatter, ReplyFormatOptions},
1616
manager::{
1717
err_msg_json_expected, err_msg_json_path_doesnt_exist_with_param, AddUpdateInfo,
1818
SetUpdateInfo, UpdateInfo,
1919
},
20-
redisjson::{normalize_arr_indices, Format, Path, SetOptions},
20+
redisjson::{normalize_arr_indices, Path, ReplyFormat, SetOptions},
2121
};
2222

2323
pub struct KeyValue<'a, V: SelectValue> {
@@ -98,7 +98,7 @@ impl<'a, V: SelectValue + 'a> KeyValue<'a, V> {
9898
Ok(results)
9999
}
100100

101-
pub fn serialize_object<O: Serialize>(o: &O, format: &FormatOptions) -> String {
101+
pub fn serialize_object<O: Serialize>(o: &O, format: &ReplyFormatOptions) -> String {
102102
// When using the default formatting, we can use serde_json's default serializer
103103
if format.no_formatting() {
104104
serde_json::to_string(o).unwrap()
@@ -113,7 +113,7 @@ impl<'a, V: SelectValue + 'a> KeyValue<'a, V> {
113113
fn to_json_multi(
114114
&self,
115115
paths: &mut Vec<Path>,
116-
format: &FormatOptions,
116+
format: &ReplyFormatOptions,
117117
is_legacy: bool,
118118
) -> Result<RedisValue, Error> {
119119
// TODO: Creating a temp doc here duplicates memory usage. This can be very memory inefficient.
@@ -177,7 +177,11 @@ impl<'a, V: SelectValue + 'a> KeyValue<'a, V> {
177177
Ok(res)
178178
}
179179

180-
fn to_resp3(&self, paths: &mut Vec<Path>, format: &FormatOptions) -> Result<RedisValue, Error> {
180+
fn to_resp3(
181+
&self,
182+
paths: &mut Vec<Path>,
183+
format: &ReplyFormatOptions,
184+
) -> Result<RedisValue, Error> {
181185
let results = paths
182186
.drain(..)
183187
.map(|path: Path| self.to_resp3_path(&path, format))
@@ -186,7 +190,7 @@ impl<'a, V: SelectValue + 'a> KeyValue<'a, V> {
186190
Ok(RedisValue::Array(results))
187191
}
188192

189-
pub fn to_resp3_path(&self, path: &Path, format: &FormatOptions) -> RedisValue {
193+
pub fn to_resp3_path(&self, path: &Path, format: &ReplyFormatOptions) -> RedisValue {
190194
compile(path.get_path()).map_or_else(
191195
|_| RedisValue::Array(vec![]),
192196
|q| Self::values_to_resp3(&calc_once(q, self.val), format),
@@ -196,7 +200,7 @@ impl<'a, V: SelectValue + 'a> KeyValue<'a, V> {
196200
fn to_json_single(
197201
&self,
198202
path: &str,
199-
format: &FormatOptions,
203+
format: &ReplyFormatOptions,
200204
is_legacy: bool,
201205
) -> Result<RedisValue, Error> {
202206
let res = if is_legacy {
@@ -210,16 +214,16 @@ impl<'a, V: SelectValue + 'a> KeyValue<'a, V> {
210214
Ok(res)
211215
}
212216

213-
fn values_to_resp3(values: &[&V], format: &FormatOptions) -> RedisValue {
217+
fn values_to_resp3(values: &[&V], format: &ReplyFormatOptions) -> RedisValue {
214218
values
215219
.iter()
216220
.map(|v| Self::value_to_resp3(v, format))
217221
.collect::<Vec<RedisValue>>()
218222
.into()
219223
}
220224

221-
pub fn value_to_resp3(value: &V, format: &FormatOptions) -> RedisValue {
222-
if format.format == Format::EXPAND {
225+
pub fn value_to_resp3(value: &V, format: &ReplyFormatOptions) -> RedisValue {
226+
if format.format == ReplyFormat::EXPAND {
223227
match value.get_type() {
224228
SelectValueType::Null => RedisValue::Null,
225229
SelectValueType::Bool => RedisValue::Bool(value.get_bool()),
@@ -260,11 +264,8 @@ impl<'a, V: SelectValue + 'a> KeyValue<'a, V> {
260264
pub fn to_json(
261265
&self,
262266
paths: &mut Vec<Path>,
263-
format: &FormatOptions,
267+
format: &ReplyFormatOptions,
264268
) -> Result<RedisValue, Error> {
265-
if format.format == Format::BSON {
266-
return Err("ERR Soon to come...".into());
267-
}
268269
let is_legacy = !paths.iter().any(|p| !p.is_legacy());
269270

270271
// If we're using RESP3, we need to reply with an array of values
@@ -351,12 +352,20 @@ impl<'a, V: SelectValue + 'a> KeyValue<'a, V> {
351352
}
352353
}
353354

354-
pub fn to_string_single(&self, path: &str, format: &FormatOptions) -> Result<String, Error> {
355+
pub fn to_string_single(
356+
&self,
357+
path: &str,
358+
format: &ReplyFormatOptions,
359+
) -> Result<String, Error> {
355360
let result = self.get_first(path)?;
356361
Ok(Self::serialize_object(&result, format))
357362
}
358363

359-
pub fn to_string_multi(&self, path: &str, format: &FormatOptions) -> Result<String, Error> {
364+
pub fn to_string_multi(
365+
&self,
366+
path: &str,
367+
format: &ReplyFormatOptions,
368+
) -> Result<String, Error> {
360369
let results = self.get_values(path)?;
361370
Ok(Self::serialize_object(&results, format))
362371
}

0 commit comments

Comments
 (0)