Skip to content

Commit ef5ebd1

Browse files
ldanilekConvex, Inc.
authored andcommitted
[CX-6011] support empty string as object FieldName (#24179)
lift restriction on ConvexObject keys being nonempty. Note this PR does not affect IdentifierFieldName at all, just FieldName. as discussed in Slack, the main blocker for this change is making sure it works in the dashboard. This unblocks customers from importing objects with empty string fields. I don't think we can easily make this safe to push in any order, because npm-packages/convex/src/value is shared by dashboard client-side and system udfs, and crates/convex/sync_types is shared by backend, funrun, and rust client. worst case scenario seems fine to me though, since it involves dashboard throwing clear errors ("Field name cannot be empty") just from a different layer -- from the system udf instead of from typechecking. And errors only happen if the developer tries to use empty string as a key. @atrakh pls check dashboard @sujayakar to make sure the backend changes make sense I tested that you can Add Documents in the dashboard and the field shows up. ![Screenshot 2024-04-02 at 1 33 48 PM](https://github.com/get-convex/convex/assets/4319355/0c23ad32-f269-46d5-9b84-590f748a0b6c) GitOrigin-RevId: 921bdd9281d040a8960d9d891e0e2eb8284cd130
1 parent e655062 commit ef5ebd1

File tree

9 files changed

+113
-43
lines changed

9 files changed

+113
-43
lines changed

crates/convex/sync_types/src/identifier.rs

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -82,24 +82,12 @@ pub fn is_valid_field_name(s: &str) -> bool {
8282
}
8383

8484
fn check_valid_field_name_inner(s: &str) -> Result<(), String> {
85-
let mut chars = s.chars();
86-
match chars.next() {
87-
Some('$') => {
88-
return Err(format!(
89-
"Field name {s} starts with '$', which is reserved."
90-
))
91-
},
92-
Some(c) => {
93-
if !c.is_ascii() || c.is_ascii_control() {
94-
return Err(format!(
95-
"Field name {s} has invalid character '{c}': Field names can only contain \
96-
non-control ASCII characters"
97-
));
98-
}
99-
},
100-
None => return Err(format!("Field name cannot be empty")),
101-
};
102-
for c in chars {
85+
if s.starts_with('$') {
86+
return Err(format!(
87+
"Field name {s} starts with '$', which is reserved."
88+
));
89+
}
90+
for c in s.chars() {
10391
if !c.is_ascii() || c.is_ascii_control() {
10492
return Err(format!(
10593
"Field name {s} has invalid character '{c}': Field names can only contain \

crates/isolate/src/tests/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ mod source_maps;
3131
mod system_udfs;
3232
mod unicode;
3333
mod user_error;
34+
mod values;
3435
mod vector_search;
3536

3637
pub fn assert_contains(error: &impl Display, expected: &str) {

crates/isolate/src/tests/values.rs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
use common::{
2+
assert_obj,
3+
value::ConvexValue,
4+
};
5+
use must_let::must_let;
6+
use runtime::testing::TestRuntime;
7+
use value::assert_val;
8+
9+
use crate::test_helpers::UdfTest;
10+
11+
#[convex_macro::test_runtime]
12+
async fn test_bigint(rt: TestRuntime) -> anyhow::Result<()> {
13+
let t = UdfTest::default(rt).await?;
14+
let value = t.query("values:intQuery", assert_obj!()).await?;
15+
must_let!(let ConvexValue::Int64(v) = value);
16+
assert_eq!(v, 1);
17+
Ok(())
18+
}
19+
20+
#[convex_macro::test_runtime]
21+
async fn test_empty_key(rt: TestRuntime) -> anyhow::Result<()> {
22+
// Check that an object with an empty key round trips through mutation and
23+
// query.
24+
let t = UdfTest::default(rt).await?;
25+
let id = t
26+
.mutation("values:insertObject", assert_obj!("obj" => {"" => "hi"}))
27+
.await?;
28+
let value = t.query("values:getObject", assert_obj!("id" => id)).await?;
29+
must_let!(let ConvexValue::Object(o) = value);
30+
assert_eq!(o.len(), 3);
31+
assert_eq!(o.get("").unwrap().clone(), assert_val!("hi"));
32+
Ok(())
33+
}

crates/local_backend/src/public_api.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -471,7 +471,7 @@ mod tests {
471471
http_format_tester(
472472
rt,
473473
"/api/query",
474-
"bigint:intQuery",
474+
"values:intQuery",
475475
json!({}),
476476
None,
477477
Err("RequiresFormatParam"),
@@ -484,7 +484,7 @@ mod tests {
484484
http_format_tester(
485485
rt,
486486
"/api/query",
487-
"bigint:intQuery",
487+
"values:intQuery",
488488
json!({}),
489489
Some("json"),
490490
Ok(json!("1")),
@@ -497,7 +497,7 @@ mod tests {
497497
http_format_tester(
498498
rt,
499499
"/api/mutation",
500-
"bigint:intMutation",
500+
"values:intMutation",
501501
json!({}),
502502
None,
503503
Err("RequiresFormatParam"),
@@ -510,7 +510,7 @@ mod tests {
510510
http_format_tester(
511511
rt,
512512
"/api/mutation",
513-
"bigint:intMutation",
513+
"values:intMutation",
514514
json!({}),
515515
Some("json"),
516516
Ok(json!("1")),
@@ -523,7 +523,7 @@ mod tests {
523523
http_format_tester(
524524
rt,
525525
"/api/action",
526-
"bigint:intAction",
526+
"values:intAction",
527527
json!({}),
528528
None,
529529
Err("RequiresFormatParam"),
@@ -536,7 +536,7 @@ mod tests {
536536
http_format_tester(
537537
rt,
538538
"/api/action",
539-
"bigint:intAction",
539+
"values:intAction",
540540
json!({}),
541541
Some("json"),
542542
Ok(json!("1")),

crates/value/src/tests.rs

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
1-
use std::mem;
1+
use std::{
2+
mem,
3+
ops::Deref,
4+
};
25

36
use crate::{
47
assert_obj,
8+
obj,
59
ConvexObject,
610
ConvexValue,
711
InternalId,
@@ -130,3 +134,35 @@ fn test_shallow_merge() -> anyhow::Result<()> {
130134

131135
Ok(())
132136
}
137+
138+
#[test]
139+
fn test_object_keys() -> anyhow::Result<()> {
140+
let obj = assert_obj!(
141+
"name" => "me",
142+
);
143+
assert_eq!(
144+
obj.keys().map(Deref::deref).collect::<Vec<_>>(),
145+
vec!["name"]
146+
);
147+
let empty_string_key = assert_obj!(
148+
"" => "empty",
149+
);
150+
assert_eq!(
151+
empty_string_key
152+
.keys()
153+
.map(Deref::deref)
154+
.collect::<Vec<_>>(),
155+
vec![""]
156+
);
157+
let control_char_key: anyhow::Result<_> = try { obj!("\t" => "tab")? };
158+
assert!(control_char_key
159+
.unwrap_err()
160+
.to_string()
161+
.contains("Field names can only contain non-control ASCII characters"));
162+
let dollar_sign_key: anyhow::Result<_> = try { obj!("$id" => "tab")? };
163+
assert!(dollar_sign_key
164+
.unwrap_err()
165+
.to_string()
166+
.contains("Field name $id starts with '$', which is reserved"));
167+
Ok(())
168+
}

npm-packages/convex/src/values/value.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,6 @@ export const base64ToBigInt = (DataView.prototype as any).getBigInt64
153153
const MAX_IDENTIFIER_LEN = 1024;
154154

155155
function validateObjectField(k: string) {
156-
if (k.length === 0) {
157-
throw new Error("Empty field names are disallowed.");
158-
}
159156
if (k.length > MAX_IDENTIFIER_LEN) {
160157
throw new Error(
161158
`Field name ${k} exceeds maximum field name length ${MAX_IDENTIFIER_LEN}.`,

npm-packages/udf-tests/convex/_generated/api.d.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import type * as asyncTests from "../asyncTests.js";
2222
import type * as auth from "../auth.js";
2323
import type * as basic from "../basic.js";
2424
import type * as benches from "../benches.js";
25-
import type * as bigint from "../bigint.js";
2625
import type * as creationTime from "../creationTime.js";
2726
import type * as crons from "../crons.js";
2827
import type * as crons_error from "../crons_error.js";
@@ -74,6 +73,7 @@ import type * as sourceMaps from "../sourceMaps.js";
7473
import type * as sync from "../sync.js";
7574
import type * as unicode from "../unicode.js";
7675
import type * as userError from "../userError.js";
76+
import type * as values from "../values.js";
7777
import type * as vector_search from "../vector_search.js";
7878

7979
/**
@@ -93,7 +93,6 @@ declare const fullApi: ApiFromModules<{
9393
auth: typeof auth;
9494
basic: typeof basic;
9595
benches: typeof benches;
96-
bigint: typeof bigint;
9796
creationTime: typeof creationTime;
9897
crons: typeof crons;
9998
crons_error: typeof crons_error;
@@ -145,6 +144,7 @@ declare const fullApi: ApiFromModules<{
145144
sync: typeof sync;
146145
unicode: typeof unicode;
147146
userError: typeof userError;
147+
values: typeof values;
148148
vector_search: typeof vector_search;
149149
}>;
150150
export declare const api: FilterApi<

npm-packages/udf-tests/convex/bigint.ts

Lines changed: 0 additions & 13 deletions
This file was deleted.
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import { mutation, query, action } from "./_generated/server";
2+
import { v } from "convex/values";
3+
4+
export const intQuery = query(async () => {
5+
return 1n;
6+
});
7+
8+
export const intMutation = mutation(async () => {
9+
return 1n;
10+
});
11+
12+
export const intAction = action(async () => {
13+
return 1n;
14+
});
15+
16+
export const insertObject = mutation({
17+
args: { obj: v.any() },
18+
handler: async (ctx, { obj }) => {
19+
return ctx.db.insert("test", obj);
20+
},
21+
});
22+
23+
export const getObject = query({
24+
args: { id: v.id("test") },
25+
handler: async (ctx, { id }) => {
26+
return ctx.db.get(id);
27+
},
28+
});

0 commit comments

Comments
 (0)