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

Inconsistent interval boudaries check #8878

Closed
DifferentialOrange opened this issue Jul 14, 2023 · 8 comments · Fixed by #8882
Closed

Inconsistent interval boudaries check #8878

DifferentialOrange opened this issue Jul 14, 2023 · 8 comments · Fixed by #8882
Labels
1sp bug Something isn't working teamE

Comments

@DifferentialOrange
Copy link
Contributor

DifferentialOrange commented Jul 14, 2023

datetime.interval allowed range should include boundaries, yet it doesn't.

tarantool> datetime.interval.new{year=11759221}
---
- error: 'builtin/box/console.lua:415: value 11759221 of year is out of allowed range
    [-11759221, 11759221]'
...

Square brackets imply boundaries inclusion, yet the boundaries are excluded.

if v > -max and v < max then
return v
end
error(('value %s of %s is out of allowed range [%s, %s]'):
format(v, txt, -max, max), 4)

At the same time, datetime range check

if extra == v or (v >= from and v <= to) then

and C interval range check

return (v < from) ? -1 : (v > to ? +1 : 0);

include boundaries, as expected.

@DifferentialOrange DifferentialOrange added the bug Something isn't working label Jul 14, 2023
@DifferentialOrange
Copy link
Contributor Author

Addition and subtraction also respect boundaries

tarantool> datetime.interval.new{year = 11759220} + datetime.interval.new{year = 1}
---
- +11759221 years
...

tarantool> datetime.interval.new{year = 11759220} + datetime.interval.new{year = 2}
---
- error: addition moves value 11759222 of year out of allowed range [-11759221, 11759221]
...

tarantool> datetime.interval.new{year = -11759220} - datetime.interval.new{year = 1}
---
- -11759221 years
...

tarantool> datetime.interval.new{year = -11759220} - datetime.interval.new{year = 2}
---
- error: subtraction moves value -11759222 of year out of allowed range [-11759221,
    11759221]
...

DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this issue Jul 14, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See tarantool#8878 for more info.)

Closes tarantool#8878

NO_DOC=small bug fix
DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this issue Jul 14, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See tarantool#8878 for more info.)

Closes tarantool#8878

NO_DOC=small bug fix
DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this issue Jul 14, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See tarantool#8878 for more info.)

Closes tarantool#8878

NO_DOC=small bug fix
@ArtDu
Copy link

ArtDu commented Jul 14, 2023

More strange behaviour examples:
image
image

@DifferentialOrange
Copy link
Contributor Author

DifferentialOrange commented Jul 17, 2023

More strange behaviour examples

I had filed a separate issue, follow #8887

DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this issue Jul 17, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See tarantool#8878 for more info.)

Closes tarantool#8878

NO_DOC=small bug fix
DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this issue Jul 17, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See tarantool#8878 for more info.)

Closes tarantool#8878

NO_DOC=small bug fix
DifferentialOrange added a commit to tarantool/tarantool-python that referenced this issue Jul 17, 2023
Before this patch, any value was allowed for interval attributes. Now
we use the same rules as in Tarantool. A couple of issues were met while
developing this patch, follow [1, 2] for core updates.

1. tarantool/tarantool#8878
2. tarantool/tarantool#8887
DifferentialOrange added a commit to tarantool/tarantool-python that referenced this issue Jul 18, 2023
Before this patch, any value was allowed for interval attributes. Now
we use the same rules as in Tarantool. A couple of issues were met while
developing this patch, follow [1, 2] for core updates.

1. tarantool/tarantool#8878
2. tarantool/tarantool#8887
DifferentialOrange added a commit to tarantool/tarantool-python that referenced this issue Jul 18, 2023
Before this patch, any value was allowed for interval attributes. Now
we use the same rules as in Tarantool. A couple of issues were met while
developing this patch, follow [1, 2] for core updates.

1. tarantool/tarantool#8878
2. tarantool/tarantool#8887
DifferentialOrange added a commit to tarantool/tarantool-python that referenced this issue Jul 18, 2023
Before this patch, any value was allowed for interval attributes. Now
we use the same rules as in Tarantool. A couple of issues were met while
developing this patch, follow [1, 2] for core updates.

1. tarantool/tarantool#8878
2. tarantool/tarantool#8887
DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this issue Jul 18, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See tarantool#8878 for more info.)

Closes tarantool#8878

NO_DOC=small bug fix
DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this issue Jul 19, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See tarantool#8878 for more info.)

Closes tarantool#8878

NO_DOC=small bug fix
igormunkin pushed a commit that referenced this issue Jul 19, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See #8878 for more info.)

Closes #8878

NO_DOC=small bug fix
igormunkin pushed a commit that referenced this issue Jul 19, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See #8878 for more info.)

Closes #8878

NO_DOC=small bug fix

(cherry picked from commit b2a001c)
igormunkin pushed a commit that referenced this issue Jul 19, 2023
Before this patch, one couldn't create new datetime interval with
boundary value from Lua. At the same time, it was possible to create
such interval from Lua through addition and subtraction. C range
verification allow to create boundary value intervals, error message
also implies that they should be allowed. (See #8878 for more info.)

Closes #8878

NO_DOC=small bug fix

(cherry picked from commit b2a001c)
@ArtDu
Copy link

ArtDu commented Jul 21, 2023

More strange behaviour examples: image image

@DifferentialOrange
So the solution hasn't fixed the second picture problem. We can pass from the connector more than limit if connector didn't check boundaries. For example upper limit for year field is 11759221, but I can pass int32. Does Tarantool have to control it?

@DifferentialOrange
Copy link
Contributor Author

More strange behaviour examples: image image

@DifferentialOrange So the solution hasn't fixed the second picture problem. We can pass from the connector more than limit if connector didn't check boundaries. For example upper limit for year field is 11759221, but I can pass int32. Does Tarantool have to control it?

And it wasn't expected to, follow #8887 issue and #8888 patch

@ArtDu
Copy link

ArtDu commented Jul 21, 2023

And it wasn't expected to, follow #8887 issue and #8888 patch

Correct me if I'm wrong, but #8888 fix only int32 limit problem but not problem that you can pass more than field limit value(not int32 limit).

image

@DifferentialOrange
Copy link
Contributor Author

DifferentialOrange commented Jul 21, 2023

And it wasn't expected to, follow #8887 issue and #8888 patch

Correct me if I'm wrong, but #8888 fix only int32 limit problem but not problem that you can pass more than field limit value(not int32 limit).
image

Yeah, it doesn't do anything with encode validation. File a separate issue if you wish.

@ArtDu
Copy link

ArtDu commented Jul 21, 2023

Yeah, it doesn't do anything with encode validation. File a separate issue if you wish.

I'll create a ticket. I'm just curious suddenly may you have a plan or a ticket to fix it somewhere

upd: #8905

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1sp bug Something isn't working teamE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants