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.

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 errors caused by numeric IDs that are too big for JS numbers #1153

Merged
merged 1 commit into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,12 @@ private static ValueDisplayField buildRelationshipField(

public static Literal fromApiObject(ApiLiteral apiLiteral) {
return switch (apiLiteral.getDataType()) {
case INT64 -> Literal.forInt64(apiLiteral.getValueUnion().getInt64Val());
// JavaScript can't handle the full int64 range when parsing JSON, so parse them as strings.
case INT64 ->
Literal.forInt64(
apiLiteral.getValueUnion().getInt64Val() != null
? Long.parseLong(apiLiteral.getValueUnion().getInt64Val())
: null);
case STRING -> Literal.forString(apiLiteral.getValueUnion().getStringVal());
case BOOLEAN -> Literal.forBoolean(apiLiteral.getValueUnion().isBoolVal());
case DATE -> Literal.forDate(apiLiteral.getValueUnion().getDateVal());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ public static ApiLiteral toApiObject(Literal literal) {
new ApiLiteral().dataType(ApiDataType.fromValue(literal.getDataType().name()));
return switch (literal.getDataType()) {
case INT64 ->
apiLiteral.valueUnion(new ApiLiteralValueUnion().int64Val(literal.getInt64Val()));
// JavaScript can't handle the full int64 range when parsing JSON, so send them as
// strings.
apiLiteral.valueUnion(
new ApiLiteralValueUnion().int64Val(literal.getInt64Val().toString()));
case STRING ->
apiLiteral.valueUnion(new ApiLiteralValueUnion().stringVal(literal.getStringVal()));
case BOOLEAN ->
Expand Down
4 changes: 2 additions & 2 deletions service/src/main/resources/api/service_openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1522,8 +1522,8 @@ components:
boolVal:
type: boolean
int64Val:
type: integer
format: int64
# Transfer int64s as strings to avoid overflowing JavaScript's number precision.
type: string
stringVal:
type: string
dateVal:
Expand Down
2 changes: 1 addition & 1 deletion ui/codegen.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ openapi-generator-cli generate -i ../service/src/main/resources/api/service_open
# Generate plugin proto code.
rm -rf src/proto
mkdir src/proto
npx protoc --proto_path=../underlay/src/main/proto/ --plugin=./node_modules/.bin/protoc-gen-ts_proto --ts_proto_out=./src/proto/ --ts_proto_opt=esModuleInterop=true --ts_proto_opt=outputClientImpl=false ../underlay/src/main/proto/criteriaselector/dataschema/*.proto ../underlay/src/main/proto/criteriaselector/configschema/*.proto ../underlay/src/main/proto/viz/*.proto
npx protoc --proto_path=../underlay/src/main/proto/ --plugin=./node_modules/.bin/protoc-gen-ts_proto --ts_proto_out=./src/proto/ --ts_proto_opt=esModuleInterop=true --ts_proto_opt=outputClientImpl=false --ts_proto_opt=forceLong=bigint ../underlay/src/main/proto/criteriaselector/dataschema/*.proto ../underlay/src/main/proto/criteriaselector/configschema/*.proto ../underlay/src/main/proto/viz/*.proto
2 changes: 1 addition & 1 deletion ui/src/addByCode.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ export function AddByCode() {
return undefined;
}

const item = data[id]?.data as LookupEntryItem;
const item = data.get(id)?.data as LookupEntryItem;
if (!item) {
return undefined;
}
Expand Down
2 changes: 1 addition & 1 deletion ui/src/addCohort.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export function AddCohort() {
return undefined;
}

const cohortData = data[id].data as CohortData;
const cohortData = data.get(id)?.data as CohortData;
if (!cohortData) {
return undefined;
}
Expand Down
27 changes: 13 additions & 14 deletions ui/src/addCriteria.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import Loading from "components/loading";
import { Search } from "components/search";
import {
TreeGrid,
TreeGridData,
TreeGridId,
TreeGridItem,
TreeGridRowData,
Expand Down Expand Up @@ -353,9 +352,9 @@ function AddCriteria(props: AddCriteriaProps) {

const search = useCallback(async () => {
const children: DataKey[] = [];
const data: TreeGridData = {
root: { data: {}, children },
};
const data = new Map<TreeGridId, CriteriaItem>([
["root", { data: {}, children }],
]);

if (query) {
const res = await searchCriteria(
Expand All @@ -377,13 +376,13 @@ function AddCriteria(props: AddCriteriaProps) {
},
entry: entry,
};
data[key] = item;
data.set(key, item);
});
}

return data;
}, [underlaySource, query, selectedOptions, optionsMap]);
const searchState = useSWRImmutable<TreeGridData>(
const searchState = useSWRImmutable<Map<TreeGridId, CriteriaItem>>(
{
component: "AddCriteria",
underlayName: underlay.name,
Expand Down Expand Up @@ -523,7 +522,7 @@ function AddCriteria(props: AddCriteriaProps) {
{!!query ? (
<Paper>
<Loading status={searchState}>
{!searchState.data?.root?.children?.length ? (
{!searchState.data?.get("root")?.children?.length ? (
<Empty
minHeight="300px"
image={emptyImage}
Expand All @@ -532,7 +531,7 @@ function AddCriteria(props: AddCriteriaProps) {
) : (
<TreeGrid
columns={columns}
data={searchState.data ?? {}}
data={searchState.data ?? new Map()}
rowCustomization={(
id: TreeGridId,
rowData: TreeGridRowData
Expand All @@ -541,11 +540,11 @@ function AddCriteria(props: AddCriteriaProps) {
return undefined;
}

const item = searchState.data[id] as CriteriaItem;
const option = optionsMap.get(item.entry.source);
if (!option) {
const item = searchState.data.get(id);
const option = optionsMap.get(item?.entry?.source ?? "");
if (!option || !item?.entry?.source) {
throw new Error(
`Item source "${item.entry.source}" doesn't match any criteria config ID.`
`Item source "${item?.entry?.source}" doesn't match any criteria config ID.`
);
}

Expand All @@ -556,7 +555,7 @@ function AddCriteria(props: AddCriteriaProps) {
<GridLayout colAlign="center">
<Button
data-testid={rowData[searchConfig.columns[0].key]}
onClick={() => onClick(option, item.entry.data)}
onClick={() => onClick(option, item.entry?.data)}
variant="outlined"
>
Add
Expand All @@ -577,5 +576,5 @@ function AddCriteria(props: AddCriteriaProps) {
}

type CriteriaItem = TreeGridItem & {
entry: MergedItem<DataEntry>;
entry?: MergedItem<DataEntry>;
};
19 changes: 11 additions & 8 deletions ui/src/cohortReview/cohortReview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ function AnnotationComponent(props: {

// TODO(tjennison): Expand handling of older and newer revisions and improve
// their UI once the API is updated.
const values = props.instance?.annotations?.[props.annotation.id];
const values = props.instance?.annotations?.get(props.annotation.id);
const currentValue = values?.find((v) => v.current)?.value;
const latestValue = values?.[values?.length]?.value;

Expand All @@ -339,9 +339,12 @@ function AnnotationComponent(props: {
props.instance.data.key
),
(instance: ReviewInstance) => {
instance.annotations[props.annotation.id] = instance.annotations[
props.annotation.id
]?.filter((v) => !v.current);
instance.annotations.set(
props.annotation.id,
(instance.annotations.get(props.annotation.id) ?? []).filter(
(v) => !v.current
)
);
}
);
} else {
Expand Down Expand Up @@ -432,17 +435,17 @@ function createUpdateCurrentValue(
value: DataValue
) {
const annotations = instance.annotations;
const cur = annotations[annotationId]?.find((v) => v.current);
const cur = annotations.get(annotationId)?.find((v) => v.current);
if (cur) {
cur.value = value;
} else {
annotations[annotationId] = [
...(annotations[annotationId] ?? []),
annotations.set(annotationId, [
...(annotations.get(annotationId) ?? []),
{
current: true,
instanceId: instance.data.key,
value: value,
},
];
]);
}
}
4 changes: 2 additions & 2 deletions ui/src/cohortReview/cohortReviewList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
TreeGridId,
useArrayAsTreeGridData,
} from "components/treegrid";
import { Annotation, AnnotationType, CohortReview } from "data/source";
import { AnnotationType, CohortReview } from "data/source";
import { useStudySource } from "data/studySourceContext";
import { useUnderlaySource } from "data/underlaySourceContext";
import deepEqual from "deep-equal";
Expand Down Expand Up @@ -692,7 +692,7 @@ function Annotations() {
return undefined;
}

const annotation = data[id].data as Annotation;
const annotation = data.get(id)?.data;
if (!annotation) {
return undefined;
}
Expand Down
15 changes: 8 additions & 7 deletions ui/src/cohortReview/participantsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,7 @@ function ParticipantsList(props: ParticipantsListProps) {
);

const data = useMemo(() => {
const data: TreeGridData = {
root: { data: {}, children: [] },
};
const data: TreeGridData = new Map([["root", { data: {}, children: [] }]]);

instancesState.data
?.slice(
Expand All @@ -136,13 +134,16 @@ function ParticipantsList(props: ParticipantsListProps) {
)
.forEach((instance) => {
const key = instance.data.key;
data[key] = { data: { ...instance.data } };
data.root?.children?.push(key);
data.set(key, { data: { ...instance.data } });
data.get("root")?.children?.push(key);

annotationsState.data?.forEach((a) => {
const values = instance.annotations[a.id];
const values = instance.annotations.get(a.id);
if (values) {
data[key].data[`t_${a.id}`] = values[values.length - 1].value;
const valueData = data.get(key)?.data;
if (valueData) {
valueData[`t_${a.id}`] = values[values.length - 1].value;
}
}
});
});
Expand Down
21 changes: 11 additions & 10 deletions ui/src/cohortReview/plugins/occurrenceTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,10 @@ function OccurrenceTable({ id, config }: { id: string; config: Config }) {

const data = useMemo(() => {
const children: DataKey[] = [];
const data: TreeGridData = {
root: { data: {}, children },
};
const data: TreeGridData = new Map([["root", { data: {}, children }]]);

context.rows[config.entity]?.forEach((o) => {
data[o.key] = { data: o };
data.set(o.key, { data: o });
children.push(o.key);
});

Expand All @@ -84,22 +82,25 @@ function OccurrenceTable({ id, config }: { id: string; config: Config }) {

const sortedData = useMemo(() => {
return produce(data, (data) => {
if (!data.root?.children) {
const root = data.get("root");
if (!root) {
return;
}

data.root.children = data.root.children.filter((child) =>
root.children = (root.children ?? []).filter((child) =>
Object.entries(filterRegExps ?? {}).reduce(
(cur: boolean, [col, re]) =>
cur &&
re.test(stringifyDataValue(data[child].data[col] as DataValue)),
re.test(
stringifyDataValue(data.get(child)?.data?.[col] as DataValue)
),
true
)
);
data.root.children.sort((a, b) => {
root.children.sort((a, b) => {
for (const o of searchState.sortOrders ?? []) {
const valA = data[a].data[o.column] as DataValue | undefined;
const valB = data[b].data[o.column] as DataValue | undefined;
const valA = data.get(a)?.data?.[o.column] as DataValue | undefined;
const valB = data.get(b)?.data?.[o.column] as DataValue | undefined;
const c = compareDataValues(valA, valB);
if (c !== 0) {
return o.direction === TreeGridSortDirection.Asc ? c : -c;
Expand Down
2 changes: 1 addition & 1 deletion ui/src/cohortReview/plugins/textSearch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ function TextSearch({ id, config }: { id: string; config: Config }) {
>
<Stack spacing={1} alignItems="stretch">
{entities.map((o) => (
<Paper key={o.key} sx={{ overflow: "hidden" }}>
<Paper key={String(o.key)} sx={{ overflow: "hidden" }}>
<Stack
alignItems="flex-start"
sx={{
Expand Down
67 changes: 42 additions & 25 deletions ui/src/components/treegrid.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ import CheckBoxIcon from "@mui/icons-material/CheckBox";
import IconButton from "@mui/material/IconButton";
import { render, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { TreeGrid, TreeGridData, TreeGridId } from "components/treegrid";
import {
TreeGrid,
TreeGridData,
TreeGridId,
TreeGridItem,
} from "components/treegrid";
import React from "react";

test("Table renders correctly", async () => {
Expand All @@ -27,33 +32,45 @@ test("Table renders correctly", async () => {
},
];

const data: TreeGridData = {
root: {
data: {},
children: [1, 3],
},
1: {
data: {
col1: "1-col1",
col2: "1-col2",
const data = new Map<TreeGridId, TreeGridItem>([
[
"root",
{
data: {},
children: [1, 3],
},
children: [2],
},
2: {
data: {
col1: "2-col1",
col2: "2-col2",
col3: "2-col3",
],
[
1,
{
data: {
col1: "1-col1",
col2: "1-col2",
},
children: [2],
},
},
3: {
data: {
col1: "3-col1",
col2: "3-col2",
col3: <AccountTreeIcon />,
],
[
2,
{
data: {
col1: "2-col1",
col2: "2-col2",
col3: "2-col3",
},
},
},
};
],
[
3,
{
data: {
col1: "3-col1",
col2: "3-col2",
col3: <AccountTreeIcon />,
},
},
],
]);

const rowCustomization = (id: TreeGridId) => {
// Add variety to the rows.
Expand Down
Loading