Fix bigint interface bugs #20757

wasimTQ · 2023-12-15T09:43:34Z

Scope

What's changed:

Can add numbers that are more than javascript safe integer
Validates the field for maximum and minimum values when editing through the input field and step up and down. ~~(But atm not possible to validate editing on the raw editor)~~

Potential Risks / Drawbacks

~~admin/settings/data-model/table/field -> interface tab Adding a maximum or minimum value in a bigInteger field is not possible. It thinks of the field as integer~~.

Review Notes / Questions

I would like to know if that all of the changes that're made doesn't affect other areas.
And how we can handle value from raw data editor
I think showing an error might be a better approach as well. But I don't know how to trigger an error for that field

Fixes #20287

changeset-bot · 2023-12-15T09:43:38Z

🦋 Changeset detected

Latest commit: db5e768

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@directus/app	Patch
@directus/api	Patch
directus	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

wasimTQ · 2023-12-15T09:43:54Z

@br41nslug @paescuj It still is a wip

wasimTQ · 2023-12-15T11:19:42Z

@paescuj I handled min and max. Can you tell me how can I trigger an error for specific field?
And also how can we handle the data inside raw editor. Or should I have the same validation in the raw editor as well?

app/src/components/v-input.vue

wasimTQ · 2023-12-16T09:15:45Z

I fixed all the things within the input for bigint calculations. You can check it once to confirm if everything works fine.
I might have missed something. So I'd like your feedback on it

wasimTQ · 2023-12-16T10:17:18Z

I hope the dropdown and radio interfaces has been fixed from these changes.

wasimTQ · 2023-12-18T09:16:38Z

@br41nslug Can you check it out? I think other than slider everything else works perfectly fine

br41nslug · 2023-12-18T11:25:53Z

app/src/__utils__/safe-integer.ts

+type numeric = number | bigint;
+
+export class SafeInteger {
+	private _value: numeric;


I dont think we need an underscore in the variable name here

Suggested change

private _value: numeric;

private value: numeric;

I did set it private so we can't modify it outside of the class. But can't have a getter with the same name as a variable name.

app/src/__utils__/safe-integer.ts

br41nslug · 2023-12-18T11:36:18Z

app/src/components/v-form/form-field-raw-editor.vue

@@ -87,6 +91,7 @@ const setRawValue = () => {
 					:disabled="disabled"
 					language="plaintext"
 					:placeholder="t('enter_raw_value')"
+					:is-big-integer="props.field.type === 'bigInteger'"


Let's try to work with BigInts as strings where possible so we limit the amount of "is big int" props added to where they are strictly required. I think we can drop a bunch of the "if big int then" logic if getJSType were to return string for bigInt instead of number.

app/src/components/v-form/v-form.vue

app/src/components/v-input.vue

wasimTQ · 2023-12-18T13:42:09Z

@br41nslug I hope I added the changes you suggested. Can you check it?

wasimTQ · 2023-12-18T15:29:57Z

@br41nslug Can you check it one more time and let me know if you're satisfied with the changes I made.

Also it already became a huge PR so I can work on slider in a seperate PR if you like

br41nslug · 2023-12-18T15:32:10Z

Will put it on the list for tomorrow :D Yeah the slider should definitely be in a separate PR the goal is to keep PRs as small and to the point as possible

wasimTQ · 2023-12-19T12:42:36Z

@br41nslug Can you review it now please?

br41nslug · 2023-12-19T15:54:35Z

app/src/interfaces/_system/system-raw-editor/system-raw-editor.vue

+				if (safeInt.isInvalid) {
+					inputRef.value?.focus();
+					codemirror?.setValue(content.substring(0, content.length - 1));
+					return;
+				}


Does this recursively set the value? probably a better approach to emit null if it is invalid.

No this doesn't recursively set values. As It'll be unfocused once the value is invalid.

br41nslug · 2023-12-19T15:55:36Z

app/src/interfaces/_system/system-raw-editor/system-raw-editor.vue

+	<!-- This input is exists just to unfocus the codemirror component -->
+	<input ref="inputRef" type="radio" :style="{ all: 'unset' }" />


I think we can do without this workaround.

I couldn't find any documentation on blur method first. But looks like it has a method to get the input field.

br41nslug · 2023-12-19T16:03:01Z

app/src/components/v-form/form-field-raw-editor.vue

+	if (props.field.type === 'bigInteger') {
+		return emit('setRawValue', internalValue.value);
+	}
+


This should be in the switch statement as getJSType now returns bigInteger

app/src/__utils__/safe-integer.ts

…f the global v-form

…sing the SafeInteger class

wasimTQ · 2023-12-20T07:13:03Z

@br41nslug Every changes has been made

paescuj marked this pull request as draft December 15, 2023 09:50

paescuj added 🍰 Bug Package: @directus/app labels Dec 15, 2023

br41nslug reviewed Dec 15, 2023

View reviewed changes

app/src/components/v-input.vue Outdated Show resolved Hide resolved

wasimTQ force-pushed the fix-bigint-interface-bugs branch from fd5ab30 to b2f6f96 Compare December 16, 2023 09:17

wasimTQ force-pushed the fix-bigint-interface-bugs branch from c09e0d7 to 3e3ce0e Compare December 16, 2023 12:05

br41nslug requested changes Dec 18, 2023

View reviewed changes

wasimTQ force-pushed the fix-bigint-interface-bugs branch 5 times, most recently from 2f3dfb7 to 5b2104b Compare December 18, 2023 15:23

br41nslug reviewed Dec 19, 2023

View reviewed changes

wasimTQ added 7 commits December 20, 2023 12:36

make the input accepts big integer values

6b9bffc

fix stepup and stepdown functions

4b9eaa0

set min and max fields to accept bigInteger

c43bda9

add utility class SafeInteger to work with numbers securely

5b473b6

add safe integer validations in raw editor

96c1cd4

convert all the hard coded validations with SafeInteger utility class

7d61c5a

fix validation

a6ef9c9

wasimTQ added 11 commits December 20, 2023 12:36

fix snapshot test

082f755

replace isBigInteger prop by returning the correct type

496994f

extract max and min bigint initializations in SafeInteger outside

06e296d

handle bigInteger field min and max values in the interface instead o…

e5eb654

…f the global v-form

extract SafeInteger initialization to a computed method

2d506a2

fix getJSType test

56ef295

replace computed method with reactive to calculate and make changes u…

901bd2f

…sing the SafeInteger class

avoid unnecessary watchers

870e929

replace isBigInt prop by sending the correct type as a prop

43f997e

add changeset

c9b4752

add suggested changes

db5e768

wasimTQ force-pushed the fix-bigint-interface-bugs branch from 5b2104b to db5e768 Compare December 20, 2023 07:06

This comment was marked as off-topic.

Sign in to view

rijkvanzanten removed the Package: @directus/app label Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bigint interface bugs #20757

Fix bigint interface bugs #20757

wasimTQ commented Dec 15, 2023 •

edited

Loading

changeset-bot bot commented Dec 15, 2023 •

edited

Loading

wasimTQ commented Dec 15, 2023

wasimTQ commented Dec 15, 2023

wasimTQ commented Dec 16, 2023

wasimTQ commented Dec 16, 2023

wasimTQ commented Dec 18, 2023

br41nslug Dec 18, 2023

wasimTQ Dec 18, 2023

br41nslug Dec 18, 2023

wasimTQ commented Dec 18, 2023

wasimTQ commented Dec 18, 2023

br41nslug commented Dec 18, 2023

wasimTQ commented Dec 19, 2023 •

edited

Loading

br41nslug Dec 19, 2023

wasimTQ Dec 20, 2023

br41nslug Dec 19, 2023

wasimTQ Dec 20, 2023

br41nslug Dec 19, 2023

wasimTQ commented Dec 20, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

		<!-- This input is exists just to unfocus the codemirror component -->
		<input ref="inputRef" type="radio" :style="{ all: 'unset' }" />

Fix bigint interface bugs #20757

Are you sure you want to change the base?

Fix bigint interface bugs #20757

Conversation

wasimTQ commented Dec 15, 2023 • edited Loading

Scope

Potential Risks / Drawbacks

Review Notes / Questions

changeset-bot bot commented Dec 15, 2023 • edited Loading

🦋 Changeset detected

wasimTQ commented Dec 15, 2023

wasimTQ commented Dec 15, 2023

wasimTQ commented Dec 16, 2023

wasimTQ commented Dec 16, 2023

wasimTQ commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasimTQ commented Dec 18, 2023

wasimTQ commented Dec 18, 2023

br41nslug commented Dec 18, 2023

wasimTQ commented Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasimTQ commented Dec 20, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

wasimTQ commented Dec 15, 2023 •

edited

Loading

changeset-bot bot commented Dec 15, 2023 •

edited

Loading

wasimTQ commented Dec 19, 2023 •

edited

Loading