-
Notifications
You must be signed in to change notification settings - Fork 5
fix datetime parsing issues, improve primary keys zod validation schema and error messages #204
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
base: main
Are you sure you want to change the base?
Changes from all commits
f488121
8cd11c6
6004a7f
7371169
f5d8e79
216dbaa
08877a2
21513f8
c0af43d
0c3f59b
316bb81
17ad374
14497cb
a9db8ff
bb3780d
fcfc7bf
fdee743
1fe3507
4deb5a0
d0021be
036d915
854eacf
25dfeb9
a0405e3
d58e444
9b014dd
3948256
12447ab
eba3dcc
3f5e67c
fc261e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,10 +163,31 @@ export class ColumnsService { | |
| if (!datetimeSummary?.datetimeSummary) { | ||
| throw new NotFoundException('Datetime summary NOT FOUND!'); | ||
| } | ||
|
|
||
| let datetimeDataArray: { value: Date | null }[]; | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access | ||
| if (colSeries.dtype.toString() === pl.Datetime('us').toString()) { | ||
| datetimeDataArray = dataArray.map((entry) => { | ||
| return { | ||
| value: entry.value === undefined || entry.value === null ? new Date(Math.floor(entry.value / 1000)) : null | ||
| }; | ||
| }); | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access | ||
| } else if (colSeries.dtype.toString() === pl.Datetime('ns').toString()) { | ||
| datetimeDataArray = dataArray.map((entry) => { | ||
| return { | ||
| value: | ||
| entry.value === undefined || entry.value === null ? new Date(Math.floor(entry.value / 1000000)) : null | ||
| }; | ||
| }); | ||
| } else { | ||
| datetimeDataArray = dataArray; | ||
| } | ||
|
|
||
| await this.columnModel.create({ | ||
| data: { | ||
| dataPermission: 'MANAGER', | ||
| datetimeData: dataArray, | ||
| datetimeData: datetimeDataArray, | ||
| kind: 'DATETIME', | ||
| name: colSeries.name, | ||
| nullable: colSeries.nullCount() !== 0, | ||
|
|
@@ -703,7 +724,7 @@ export class ColumnsService { | |
| data: { | ||
| datetimeColumnValidation: { | ||
| max: new Date(), | ||
| min: '1970-01-01' | ||
| min: new Date() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify appropriateness of Using Run this script to check if other datetime validations in the codebase use dynamic or static bounds: 🏁 Script executed: #!/bin/bash
# Search for datetime validation patterns
rg -nP --type=ts 'datetimeColumnValidation|Datetime.*min|Datetime.*max' -C2Length of output: 1277 I need to understand the context around line 727 better. Let me check which function contains this code and what it's trying to accomplish. Length of output: 1136 Let me check how this validation is actually used and what the intended behavior should be: Length of output: 1096 Let me search more broadly to find where this validation is actually consumed: Length of output: 733 Let me examine the context around lines 897-898 to understand the correct pattern: Length of output: 973 Fix datetime validation bounds at line 727 The validation at line 727 sets both Since 🤖 Prompt for AI Agents |
||
| }, | ||
| datetimeData: data.toArray().map((entry: Date) => { | ||
| return { value: entry }; | ||
|
|
@@ -869,10 +890,13 @@ export class ColumnsService { | |
| case 'DATETIME': | ||
| return { | ||
| count: currSeries.len() - currSeries.nullCount(), | ||
| datetimeSummary: { | ||
| max: new Date(currSeries.max() * 24 * 3600 * 1000), | ||
| min: new Date(currSeries.min() * 24 * 3600 * 1000) | ||
| }, | ||
| datetimeSummary: | ||
| currSeries.len === currSeries.nullCount | ||
| ? null | ||
| : { | ||
| max: new Date(Math.floor(currSeries.cast(pl.Datetime('ns'), true).max() / 1000000)), | ||
| min: new Date(Math.floor(currSeries.cast(pl.Datetime('ns'), true).min() / 1000000)) | ||
| }, | ||
| nullCount: currSeries.nullCount() | ||
| }; | ||
| case 'ENUM': | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -341,7 +341,7 @@ export class TabularDataService { | |||||
| if (columnIdsModifyData.has(col._id.$oid)) { | ||||||
| rows[i][col.name] = 'Hidden'; | ||||||
| } else { | ||||||
| rows[i][col.name] = entry.value.$date ? new Date(entry.value.$date).toDateString() : null; | ||||||
| rows[i][col.name] = entry.value ? new Date(entry.value.$date!).toDateString() : null; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-null assertion risks runtime failure if The condition checks Replace the non-null assertion with an explicit check: - rows[i][col.name] = entry.value ? new Date(entry.value.$date!).toDateString() : null;
+ rows[i][col.name] = entry.value?.$date ? new Date(entry.value.$date).toDateString() : null;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| } | ||||||
| }); | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,60 +6,120 @@ import { ChevronDownIcon, QuestionMarkCircleIcon, TrashIcon } from '@heroicons/r | |
| import { useQueryClient } from '@tanstack/react-query'; | ||
| import axios from 'axios'; | ||
|
|
||
| type DatasetTableProps = Omit<$TabularDataset, 'permission'> & { isManager: boolean; isProject: boolean }; | ||
| type DatasetTableProps = Omit<$TabularDataset, 'permission'> & { | ||
| isManager: boolean; | ||
| isProject: boolean; | ||
| queryKey: string; | ||
| }; | ||
|
|
||
| export const DatasetTable = (tabularDataset: DatasetTableProps) => { | ||
| const { t } = useTranslation('common'); | ||
| const addNotification = useNotificationsStore((state) => state.addNotification); | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| const handleSetColumnMetadataPermissionLevel = async (columnId: string, newPermissionLevel: $PermissionLevel) => { | ||
| await axios.patch(`/v1/datasets/column-metadata-permission/${tabularDataset.id}/${columnId}`, { | ||
| permission: newPermissionLevel | ||
| }); | ||
| await queryClient.invalidateQueries({ queryKey: ['dataset-query'] }); | ||
| addNotification({ | ||
| message: `The metadata permission level of column with Id ${columnId} has been modified`, | ||
| type: 'success' | ||
| }); | ||
| }; | ||
| const handleSetColumnMetadataPermissionLevel = useDestructiveAction( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider using mutations with tanstack query here. You can have an onSuccess callback to then invalidate and keep all the API calls in the same place. Example: export function useCreateInstrumentRecordMutation() {
const queryClient = useQueryClient();
return useMutation({
mutationFn: ({ data }: { data: $CreateInstrumentRecordData }) => {
return axios.post<$InstrumentRecord>('/v1/instrument-records', data);
},
onSuccess: async () => {
await queryClient.invalidateQueries({ queryKey: getInstrumentRecordsQueryKey() });
await queryClient.invalidateQueries({ queryKey: getProjectQueryKey() });
}
});
} |
||
| async (columnId: string, newPermissionLevel: $PermissionLevel) => { | ||
| await axios | ||
| .patch(`/v1/datasets/column-metadata-permission/${tabularDataset.id}/${columnId}`, { | ||
| permission: newPermissionLevel | ||
| }) | ||
| .then(() => { | ||
| addNotification({ | ||
| message: `The metadata permission level of column with Id ${columnId} has been modified`, | ||
| type: 'success' | ||
| }); | ||
| void queryClient.invalidateQueries({ | ||
| queryKey: [tabularDataset.queryKey] | ||
| }); | ||
| }) | ||
| .catch((error) => { | ||
| console.error(error); | ||
| addNotification({ | ||
| message: t('setColumnMetadataPermissionFailure'), | ||
| type: 'error' | ||
| }); | ||
| }); | ||
| } | ||
| ); | ||
|
|
||
| const handleSetColumnDataPermissionLevel = async (columnId: string, newPermissionLevel: $PermissionLevel) => { | ||
| await axios.patch(`/v1/datasets/column-data-permission/${tabularDataset.id}/${columnId}`, { | ||
| permission: newPermissionLevel | ||
| }); | ||
| await queryClient.invalidateQueries({ queryKey: ['dataset-query'] }); | ||
| addNotification({ | ||
| message: `The data permission level of column with Id ${columnId} has been modified`, | ||
| type: 'success' | ||
| }); | ||
| }; | ||
| const handleSetColumnDataPermissionLevel = useDestructiveAction( | ||
| async (columnId: string, newPermissionLevel: $PermissionLevel) => { | ||
| await axios | ||
| .patch(`/v1/datasets/column-data-permission/${tabularDataset.id}/${columnId}`, { | ||
| permission: newPermissionLevel | ||
| }) | ||
| .then(() => { | ||
| addNotification({ | ||
| message: `The data permission level of column with Id ${columnId} has been modified`, | ||
| type: 'success' | ||
| }); | ||
| void queryClient.invalidateQueries({ queryKey: [tabularDataset.queryKey] }); | ||
| }) | ||
| .catch((error) => { | ||
| console.error(error); | ||
| addNotification({ | ||
| message: t('setColumnDataPermissionFailure'), | ||
| type: 'error' | ||
| }); | ||
| }); | ||
| } | ||
| ); | ||
|
|
||
| const handleToggleColumnNullable = async (columnId: string) => { | ||
| await axios.patch(`/v1/datasets/column-nullable/${tabularDataset.id}/${columnId}`); | ||
| await queryClient.invalidateQueries({ queryKey: ['dataset-query'] }); | ||
| addNotification({ | ||
| message: `The nullability of column with Id ${columnId} has been modified`, | ||
| type: 'success' | ||
| }); | ||
| await axios | ||
| .patch(`/v1/datasets/column-nullable/${tabularDataset.id}/${columnId}`) | ||
| .then(() => { | ||
| addNotification({ | ||
| message: `The nullability of column with Id ${columnId} has been modified`, | ||
| type: 'success' | ||
| }); | ||
| void queryClient.invalidateQueries({ queryKey: [tabularDataset.queryKey] }); | ||
| }) | ||
|
weijietan-fc marked this conversation as resolved.
|
||
| .catch((error) => { | ||
| console.error(error); | ||
| addNotification({ | ||
| message: t('toggleColumnNullableFailure'), | ||
| type: 'error' | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| const handleChangeColumnType = async (columnId: string, type: $ColumnType) => { | ||
| await axios.patch(`/v1/datasets/column-type/${tabularDataset.id}/${columnId}`, { kind: type }); | ||
| await queryClient.invalidateQueries({ queryKey: ['dataset-query'] }); | ||
| addNotification({ | ||
| message: `The column type of column with Id ${columnId} has been modified`, | ||
| type: 'success' | ||
| }); | ||
| }; | ||
| const handleChangeColumnType = useDestructiveAction(async (columnId: string, type: $ColumnType) => { | ||
| await axios | ||
| .patch(`/v1/datasets/column-type/${tabularDataset.id}/${columnId}`, { kind: type }) | ||
| .then(() => { | ||
| addNotification({ | ||
| message: `The data type of column with Id ${columnId} has been modified`, | ||
| type: 'success' | ||
| }); | ||
| void queryClient.invalidateQueries({ queryKey: [tabularDataset.queryKey] }); | ||
| }) | ||
| .catch((error) => { | ||
| console.error(error); | ||
| addNotification({ | ||
| message: t('changeColumnDataTypeFailure'), | ||
| type: 'error' | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| const handleDeleteColumn = useDestructiveAction(async (columnId: string) => { | ||
| await axios.delete(`/v1/datasets/column/${tabularDataset.id}/${columnId}`); | ||
| await queryClient.invalidateQueries({ queryKey: ['dataset-query'] }); | ||
| addNotification({ | ||
| message: `Column with Id ${columnId} has been deleted`, | ||
| type: 'success' | ||
| }); | ||
| await axios | ||
| .delete(`/v1/datasets/column/${tabularDataset.id}/${columnId}`) | ||
| .then(() => { | ||
| addNotification({ | ||
| message: `The column with Id ${columnId} has been deleted.`, | ||
| type: 'success' | ||
| }); | ||
| void queryClient.invalidateQueries({ queryKey: [tabularDataset.queryKey] }); | ||
| }) | ||
| .catch((error) => { | ||
| console.error(error); | ||
| addNotification({ | ||
| message: t('deleteColumnFailure'), | ||
| type: 'error' | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| const getSummary = (columnName: string) => { | ||
|
|
@@ -135,7 +195,7 @@ export const DatasetTable = (tabularDataset: DatasetTableProps) => { | |
| <ChevronDownIcon className="w-3" /> | ||
| </div> | ||
| </DropdownMenu.Trigger> | ||
| <DropdownMenu.Content className="w-56"> | ||
| <DropdownMenu.Content className="w-64"> | ||
| {tabularDataset.isManager && ( | ||
| <> | ||
| <DropdownMenu.Group> | ||
|
|
||
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.
Critical: Inverted null-handling logic corrupts all datetime data
The ternary condition is backwards. Lines 172 and 180 say "if value is null/undefined, THEN divide it and create a Date, ELSE return null." This inverts the entire dataset:
new Date(Math.floor(null / 1000))→new Date(0)→ 1970-01-01nullThis was flagged in previous reviews but the fix applied the opposite logic.
Apply this fix to correct the condition:
if (colSeries.dtype.toString() === pl.Datetime('us').toString()) { datetimeDataArray = dataArray.map((entry) => { return { - value: entry.value === undefined || entry.value === null ? new Date(Math.floor(entry.value / 1000)) : null + value: entry.value === undefined || entry.value === null ? null : new Date(Math.floor(entry.value / 1000)) }; }); } else if (colSeries.dtype.toString() === pl.Datetime('ns').toString()) { datetimeDataArray = dataArray.map((entry) => { return { value: - entry.value === undefined || entry.value === null ? new Date(Math.floor(entry.value / 1000000)) : null + entry.value === undefined || entry.value === null ? null : new Date(Math.floor(entry.value / 1000000)) }; }); } else {🤖 Prompt for AI Agents