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.

Sign up for GitHub

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

Fix date-only searching. #15337

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+236 −14
Open

Fix date-only searching. #15337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
26c0861
Fix date-only searching.
samwho Jan 9, 2025
7ad5879
Create search cross product on whether we save/search with timestamp.
samwho Jan 9, 2025
f477d1b
Merge branches 'budi-8957-date-filter-equals-does-not-work' and 'mast…
samwho Jan 9, 2025
b9a5a71
Remove focus test.
samwho Jan 9, 2025
d15a5f3
Fix row.spec.ts
samwho Jan 9, 2025
8230a5d
Merge branch 'master' into budi-8957-date-filter-equals-does-not-work
mike12345567 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/backend-core/src/middleware/errorHandling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,12 @@ export async function errorHandling(ctx: any, next: any) {
}

if (environment.isTest() && ctx.headers["x-budibase-include-stacktrace"]) {
let rootErr = err
while (rootErr.cause) {
rootErr = rootErr.cause
}
// @ts-ignore
error.stack = err.stack
error.stack = rootErr.stack
Comment on lines +35 to +40
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was bothering me when tests failed. I only got shown the outermost error, which was never where the real problem was. This makes the error stack traces substantially more useful.

}

ctx.body = error
Expand Down
54 changes: 53 additions & 1 deletion packages/backend-core/src/sql/sql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,14 +816,29 @@ class InternalBuilder {
filters.oneOf,
ArrayOperator.ONE_OF,
(q, key: string, array) => {
const schema = this.getFieldSchema(key)
const values = Array.isArray(array) ? array : [array]
if (shouldOr) {
q = q.or
}
if (this.client === SqlClient.ORACLE) {
// @ts-ignore
key = this.convertClobs(key)
} else if (
this.client === SqlClient.SQL_LITE &&
schema?.type === FieldType.DATETIME &&
schema.dateOnly
) {
for (const value of values) {
if (value != null) {
q = q.or.whereLike(key, `${value.toISOString().slice(0, 10)}%`)
} else {
q = q.or.whereNull(key)
}
}
Comment on lines +832 to +838
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a shame we can't easily use whereIn here. We could, I suppose, if we expanded out, say, 2020-01-01 to ["2020-01-01", "2020-01-01T00:00:00Z"] but that felt more fraught than a bunch of LIKE queries.

return q
}
return q.whereIn(key, Array.isArray(array) ? array : [array])
return q.whereIn(key, values)
},
(q, key: string[], array) => {
if (shouldOr) {
Expand Down Expand Up @@ -882,6 +897,19 @@ class InternalBuilder {
let high = value.high
let low = value.low

if (
this.client === SqlClient.SQL_LITE &&
schema?.type === FieldType.DATETIME &&
schema.dateOnly
) {
if (high != null) {
high = `${high.toISOString().slice(0, 10)}T23:59:59.999Z`
}
if (low != null) {
low = low.toISOString().slice(0, 10)
}
}

if (this.client === SqlClient.ORACLE) {
rawKey = this.convertClobs(key)
} else if (
Expand Down Expand Up @@ -914,6 +942,7 @@ class InternalBuilder {
}
if (filters.equal) {
iterate(filters.equal, BasicOperator.EQUAL, (q, key, value) => {
const schema = this.getFieldSchema(key)
if (shouldOr) {
q = q.or
}
Expand All @@ -928,6 +957,16 @@ class InternalBuilder {
// @ts-expect-error knex types are wrong, raw is fine here
subq.whereNotNull(identifier).andWhere(identifier, value)
)
} else if (
this.client === SqlClient.SQL_LITE &&
schema?.type === FieldType.DATETIME &&
schema.dateOnly
) {
if (value != null) {
return q.whereLike(key, `${value.toISOString().slice(0, 10)}%`)
} else {
return q.whereNull(key)
}
} else {
return q.whereRaw(`COALESCE(?? = ?, FALSE)`, [
this.rawQuotedIdentifier(key),
Expand All @@ -938,6 +977,7 @@ class InternalBuilder {
}
if (filters.notEqual) {
iterate(filters.notEqual, BasicOperator.NOT_EQUAL, (q, key, value) => {
const schema = this.getFieldSchema(key)
if (shouldOr) {
q = q.or
}
Expand All @@ -959,6 +999,18 @@ class InternalBuilder {
// @ts-expect-error knex types are wrong, raw is fine here
.or.whereNull(identifier)
)
} else if (
this.client === SqlClient.SQL_LITE &&
schema?.type === FieldType.DATETIME &&
schema.dateOnly
) {
if (value != null) {
return q.not
.whereLike(key, `${value.toISOString().slice(0, 10)}%`)
.or.whereNull(key)
} else {
return q.not.whereNull(key)
}
} else {
return q.whereRaw(`COALESCE(?? != ?, TRUE)`, [
this.rawQuotedIdentifier(key),
Expand Down
12 changes: 2 additions & 10 deletions packages/backend-core/src/sql/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import environment from "../environment"
const DOUBLE_SEPARATOR = `${SEPARATOR}${SEPARATOR}`
const ROW_ID_REGEX = /^\[.*]$/g
const ENCODED_SPACE = encodeURIComponent(" ")
const ISO_DATE_REGEX = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z$/
const ISO_DATE_REGEX = /^\d{4}-\d{2}-\d{2}(?:T\d{2}:\d{2}:\d{2}.\d{3}Z)?$/
const TIME_REGEX = /^(?:\d{2}:)?(?:\d{2}:)(?:\d{2})$/

export function isExternalTableID(tableId: string) {
Expand Down Expand Up @@ -149,15 +149,7 @@ export function isInvalidISODateString(str: string) {
}

export function isValidISODateString(str: string) {
const trimmedValue = str.trim()
if (!ISO_DATE_REGEX.test(trimmedValue)) {
return false
}
let d = new Date(trimmedValue)
if (isNaN(d.getTime())) {
return false
}
return d.toISOString() === trimmedValue
return ISO_DATE_REGEX.test(str.trim())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is used in a single place, and it's where we parse incoming data to be searched on. I want both dates and date time to get parsed into Date objects, so I've modified this function to include anything that looks like 2020-01-01 or 2020-01-01T00:00:00Z.

It stood out to me that the regex we use does not consider any other timezones, not sure how big a deal that is in practice.

}

export function isValidFilter(value: any) {
Expand Down
2 changes: 1 addition & 1 deletion packages/server/src/api/routes/tests/row.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2341,7 +2341,7 @@ if (descriptions.length) {
[FieldType.ARRAY]: ["options 2", "options 4"],
[FieldType.NUMBER]: generator.natural(),
[FieldType.BOOLEAN]: generator.bool(),
[FieldType.DATETIME]: generator.date().toISOString(),
[FieldType.DATETIME]: generator.date().toISOString().slice(0, 10),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For whatever reason in this test we use a datetime field but set dateOnly=true.

[FieldType.ATTACHMENTS]: [setup.structures.basicAttachment()],
[FieldType.ATTACHMENT_SINGLE]: setup.structures.basicAttachment(),
[FieldType.FORMULA]: undefined, // generated field
Expand Down
145 changes: 145 additions & 0 deletions packages/server/src/api/routes/tests/search.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1683,6 +1683,151 @@ if (descriptions.length) {
})
})

describe("datetime - date only", () => {
describe.each([true, false])(
"saved with timestamp: %s",
saveWithTimestamp => {
describe.each([true, false])(
"search with timestamp: %s",
searchWithTimestamp => {
Comment on lines +1686 to +1692
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep I hate it too, but we're in a position where dates could have been saved into SQS with or without time components, and you can also search with or without a time component. For dateOnly fields, I think the best trade-off is to handle the cross product of all eventualities in the same way: ignore time.

const SAVE_SUFFIX = saveWithTimestamp
? "T00:00:00.000Z"
: ""
const SEARCH_SUFFIX = searchWithTimestamp
? "T00:00:00.000Z"
: ""

const JAN_1ST = `2020-01-01`
const JAN_10TH = `2020-01-10`
const JAN_30TH = `2020-01-30`
const UNEXISTING_DATE = `2020-01-03`
const NULL_DATE__ID = `null_date__id`

beforeAll(async () => {
tableOrViewId = await createTableOrView({
dateid: { name: "dateid", type: FieldType.STRING },
date: {
name: "date",
type: FieldType.DATETIME,
dateOnly: true,
},
})

await createRows([
{ dateid: NULL_DATE__ID, date: null },
{ date: `${JAN_1ST}${SAVE_SUFFIX}` },
{ date: `${JAN_10TH}${SAVE_SUFFIX}` },
])
})

describe("equal", () => {
it("successfully finds a row", async () => {
await expectQuery({
equal: { date: `${JAN_1ST}${SEARCH_SUFFIX}` },
}).toContainExactly([{ date: JAN_1ST }])
})

it("successfully finds an ISO8601 row", async () => {
await expectQuery({
equal: { date: `${JAN_10TH}${SEARCH_SUFFIX}` },
}).toContainExactly([{ date: JAN_10TH }])
})

it("finds a row with ISO8601 timestamp", async () => {
await expectQuery({
equal: { date: `${JAN_1ST}${SEARCH_SUFFIX}` },
}).toContainExactly([{ date: JAN_1ST }])
})

it("fails to find nonexistent row", async () => {
await expectQuery({
equal: {
date: `${UNEXISTING_DATE}${SEARCH_SUFFIX}`,
},
}).toFindNothing()
})
})

describe("notEqual", () => {
it("successfully finds a row", async () => {
await expectQuery({
notEqual: { date: `${JAN_1ST}${SEARCH_SUFFIX}` },
}).toContainExactly([
{ date: JAN_10TH },
{ dateid: NULL_DATE__ID },
])
})

it("fails to find nonexistent row", async () => {
await expectQuery({
notEqual: { date: `${JAN_30TH}${SEARCH_SUFFIX}` },
}).toContainExactly([
{ date: JAN_1ST },
{ date: JAN_10TH },
{ dateid: NULL_DATE__ID },
])
})
})

describe("oneOf", () => {
it("successfully finds a row", async () => {
await expectQuery({
oneOf: { date: [`${JAN_1ST}${SEARCH_SUFFIX}`] },
}).toContainExactly([{ date: JAN_1ST }])
})

it("fails to find nonexistent row", async () => {
await expectQuery({
oneOf: {
date: [`${UNEXISTING_DATE}${SEARCH_SUFFIX}`],
},
}).toFindNothing()
})
})

describe("range", () => {
it("successfully finds a row", async () => {
await expectQuery({
range: {
date: {
low: `${JAN_1ST}${SEARCH_SUFFIX}`,
high: `${JAN_1ST}${SEARCH_SUFFIX}`,
},
},
}).toContainExactly([{ date: JAN_1ST }])
})

it("successfully finds multiple rows", async () => {
await expectQuery({
range: {
date: {
low: `${JAN_1ST}${SEARCH_SUFFIX}`,
high: `${JAN_10TH}${SEARCH_SUFFIX}`,
},
},
}).toContainExactly([
{ date: JAN_1ST },
{ date: JAN_10TH },
])
})

it("successfully finds no rows", async () => {
await expectQuery({
range: {
date: {
low: `${JAN_30TH}${SEARCH_SUFFIX}`,
high: `${JAN_30TH}${SEARCH_SUFFIX}`,
},
},
}).toFindNothing()
})
})
}
)
}
)
})

isInternal &&
!isInMemory &&
describe("AI Column", () => {
Expand Down
9 changes: 9 additions & 0 deletions packages/server/src/utilities/rowProcessor/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,15 @@ export async function coreOutputProcessing(
row[property] = `${hours}:${minutes}:${seconds}`
}
}
} else if (column.type === FieldType.DATETIME && column.dateOnly) {
for (const row of rows) {
if (typeof row[property] === "string") {
row[property] = new Date(row[property])
}
if (row[property] instanceof Date) {
row[property] = row[property].toISOString().slice(0, 10)
}
}
Comment on lines +414 to +422
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We used to return date only fields with a timestamp sometimes, in a format that wasn't consistent (depended how each database returned the data, e.g. postgres would do 2020-01-01 +00:00:00). This unifies them all to return 2020-01-01.

} else if (column.type === FieldType.LINK) {
for (let row of rows) {
// if relationship is empty - remove the array, this has been part of the API for some time
Expand Down
22 changes: 21 additions & 1 deletion packages/shared-core/src/filters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,27 @@ export function runQuery<T extends Record<string, any>>(
return docValue._id === testValue
}

return docValue === testValue
if (docValue === testValue) {
return true
}

if (docValue == null && testValue != null) {
return false
}

if (docValue != null && testValue == null) {
return false
}

const leftDate = dayjs(docValue)
if (leftDate.isValid()) {
const rightDate = dayjs(testValue)
if (rightDate.isValid()) {
return leftDate.isSame(rightDate)
}
}
Comment on lines +714 to +720
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to help in-memory search match date-only fields correctly when either side is a date/datetime.


return false
}

const not =
Expand Down
Loading