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

Jump to bottom

Mathematical operations need to wrap in parenthesis #2017

Closed
2 tasks done
sgrif opened this issue Mar 19, 2019 · 0 comments
Closed
2 tasks done

Mathematical operations need to wrap in parenthesis #2017

sgrif opened this issue Mar 19, 2019 · 0 comments

Comments

@sgrif
Copy link
Member

sgrif commented Mar 19, 2019

Setup

Versions

  • Rust: any
  • Diesel: any
  • Database: any
  • Operating System any

Feature Flags

  • diesel:

Problem Description

Parenthesis inserted in Rust to override order of operations are lost in the generated SQL. This can also occur when a value is assigned to a local variable. This will at best result in an incorrect value being generated, and at worst will result in a runtime error because of incorrect types (e.g. interval * (integer - integer) is valid but interval * integer - integer is not)

What is the expected output?

14

What is the actual output?

10

Steps to reproduce

fn test(conn: &Connection) {
    let x = select(2.into_sql::<Integer>() * (3.into_sql::<Integer>() + 4))
        .get_result(conn);
    assert_eq!(Ok(14), x);
}

Checklist

  • I have already looked over the issue tracker for similar issues.
  • This issue can be reproduced on Rust's stable channel. (Your issue will be
    closed if this is not the case)
sgrif added a commit that referenced this issue Mar 19, 2019
@sgrif
Wrap all math operations in parens
d40d263 
Previously if you used local variables or parens in Rust to override the
default order of operations, we would completely lose that information
in the generated SQL. This would at best result in an unexpected value,
but at worse would cause a query like `now + 2.minutes() * (some_int +
1)` to result in a runtime error complaining that there's no operator
for `interval + integer`

I intend to backport this to 1.4

Fixes #2017
@sgrif sgrif mentioned this issue Mar 19, 2019
Merged
sgrif added a commit that referenced this issue Mar 19, 2019
@sgrif
Wrap all math operations in parens
0b6ee50 
Previously if you used local variables or parens in Rust to override the
default order of operations, we would completely lose that information
in the generated SQL. This would at best result in an unexpected value,
but at worse would cause a query like `now + 2.minutes() * (some_int +
1)` to result in a runtime error complaining that there's no operator
for `interval + integer`

I intend to backport this to 1.4

Fixes #2017
sgrif added a commit that referenced this issue Mar 19, 2019
@sgrif
Wrap all math operations in parens
1f2ab1f 
Previously if you used local variables or parens in Rust to override the
default order of operations, we would completely lose that information
in the generated SQL. This would at best result in an unexpected value,
but at worse would cause a query like `now + 2.minutes() * (some_int +
1)` to result in a runtime error complaining that there's no operator
for `interval + integer`

I intend to backport this to 1.4

Fixes #2017
@sgrif sgrif mentioned this issue Mar 19, 2019
Merged
pietgeursen pushed a commit to pietgeursen/diesel that referenced this issue May 16, 2019
@sgrif @pietgeursen
Wrap all math operations in parens
747b1ff 
Previously if you used local variables or parens in Rust to override the
default order of operations, we would completely lose that information
in the generated SQL. This would at best result in an unexpected value,
but at worse would cause a query like `now + 2.minutes() * (some_int +
1)` to result in a runtime error complaining that there's no operator
for `interval + integer`

I intend to backport this to 1.4

Fixes diesel-rs#2017
sgrif added a commit to sgrif/crates.io that referenced this issue May 30, 2019
@sgrif
Add more aggressive rate limiting for publishing new crates
676efab 
I think the limit we'll probably set to start is 1 req/10s with a burst
of 30. The error message will tell folks they can either wait for {time
until next token} or email us to get the limit increased for them. This
is limited per user instead of per ip since rotating your user is harder
than rotating your IP. It's stored in the DB since this is only for
publishing new crates, which is slow enough already that the DB load of
rate limiting there shouldn't matter.

I needed to update to Rust 1.33 to get `Duration::as_millis` (note: the
way we're using this feature causes UB if the rate limit is slower than
1 request per 292471208 years. I assume this is not a problem) I needed
to update to Diesel 1.4.2 to get a fix for
diesel-rs/diesel#2017

The algorithm used is pretty much the standard token bucket algorithm.
It's *slightly* different in how we set `tokens = max(0, tokens - 1) +
tokens_to_add` instead of `tokens = max(0, tokens_to_add + 1)`. This is
because the usual implementation checks available tokens before
subtracting them (and thus never persists if there aren't enough tokens
available). Since we're doing this in a single query, and we can *only*
return the final, persisted value, we have to change the calculation
slightly to make sure that a user who is out of tokens gets `1` back
after the rate limit.

A side effect of all of this is that our token count is actually offset
by 1. 0 means the user is not only out of tokens, but that we just tried
to take a token and couldn't. 1 means an empty bucket, and a full bucket
would technically be burst + 1. The alternative would be -1 meaning the
user is actually out of tokens, but since we only ever refill the bucket
when we're trying to take a token, we never actually persist a full
bucket. I figured a range of 0...burst made more sense than -1..burst.
bors added a commit to rust-lang/crates.io that referenced this issue May 30, 2019
@bors
Auto merge of #1681 - sgrif:sg-rate-limit-publish, r=jtgeibel
e71d82a 
Add more aggressive rate limiting for publishing new crates

This is still incomplete, but the bulk of the code has been written so I figured I'd get some eyes on it. Right now this just panics instead of returning an error if the user is out of tokens. Still left to do are:

- The two ignored test cases
- Implementing the actual error type
- Per-user burst rate overrides
- cron job to restrict the table size and clean up stale buckets (I probably won't land this in the initial PR, our users table needs to grow by 2 orders of magnitude for this to really matter -- but I do want to land it as a followup PR since I haven't tested this with cases where now - last_update is greater than a month. It should work fine but I'd rather not have this run against poorly defined semantics)

I think the limit we'll probably set to start is 1 req/10s with a burst of 30. The error message will tell folks they can either wait for {time until next token} or email us to get the limit increased for them. This is limited per user instead of per ip since rotating your user is harder than rotating your IP. It's stored in the DB since this is only for publishing new crates, which is slow enough already that the DB load of rate limiting there shouldn't matter.

I needed to update to Rust 1.33 to get `Duration::as_millis` (note: the way we're using this feature causes UB if the rate limit is slower than 1 request per 292471208 years. I assume this is not a problem)
I needed to update to Diesel 1.4.2 to get a fix for diesel-rs/diesel#2017

The algorithm used is pretty much the standard token bucket algorithm. It's *slightly* different in how we set `tokens = max(0, tokens - 1) + tokens_to_add` instead of `tokens = max(0, tokens_to_add + 1)`. This is because the usual implementation checks available tokens before subtracting them (and thus never persists if there aren't enough tokens available). Since we're doing this in a single query, and we can *only* return the final, persisted value, we have to change the calculation slightly to make sure that a user who is out of tokens gets `1` back after the rate limit.

A side effect of all of this is that our token count is actually offset by 1. 0 means the user is not only out of tokens, but that we just tried to take a token and couldn't. 1 means an empty bucket, and a full bucket would technically be burst + 1. The alternative would be -1 meaning the user is actually out of tokens, but since we only ever refill the bucket when we're trying to take a token, we never actually persist a full bucket. I figured a range of 0...burst made more sense than -1..burst.
bors added a commit to rust-lang/crates.io that referenced this issue May 30, 2019
@bors
Auto merge of #1681 - sgrif:sg-rate-limit-publish, r=jtgeibel
a6de1e1 
Add more aggressive rate limiting for publishing new crates

This is still incomplete, but the bulk of the code has been written so I figured I'd get some eyes on it. Right now this just panics instead of returning an error if the user is out of tokens. Still left to do are:

- The two ignored test cases
- Implementing the actual error type
- Per-user burst rate overrides
- cron job to restrict the table size and clean up stale buckets (I probably won't land this in the initial PR, our users table needs to grow by 2 orders of magnitude for this to really matter -- but I do want to land it as a followup PR since I haven't tested this with cases where now - last_update is greater than a month. It should work fine but I'd rather not have this run against poorly defined semantics)

I think the limit we'll probably set to start is 1 req/10s with a burst of 30. The error message will tell folks they can either wait for {time until next token} or email us to get the limit increased for them. This is limited per user instead of per ip since rotating your user is harder than rotating your IP. It's stored in the DB since this is only for publishing new crates, which is slow enough already that the DB load of rate limiting there shouldn't matter.

I needed to update to Rust 1.33 to get `Duration::as_millis` (note: the way we're using this feature causes UB if the rate limit is slower than 1 request per 292471208 years. I assume this is not a problem)
I needed to update to Diesel 1.4.2 to get a fix for diesel-rs/diesel#2017

The algorithm used is pretty much the standard token bucket algorithm. It's *slightly* different in how we set `tokens = max(0, tokens - 1) + tokens_to_add` instead of `tokens = max(0, tokens_to_add + 1)`. This is because the usual implementation checks available tokens before subtracting them (and thus never persists if there aren't enough tokens available). Since we're doing this in a single query, and we can *only* return the final, persisted value, we have to change the calculation slightly to make sure that a user who is out of tokens gets `1` back after the rate limit.

A side effect of all of this is that our token count is actually offset by 1. 0 means the user is not only out of tokens, but that we just tried to take a token and couldn't. 1 means an empty bucket, and a full bucket would technically be burst + 1. The alternative would be -1 meaning the user is actually out of tokens, but since we only ever refill the bucket when we're trying to take a token, we never actually persist a full bucket. I figured a range of 0...burst made more sense than -1..burst.
@sgrif sgrif mentioned this issue Aug 14, 2019
Closed
2 tasks
disconsented pushed a commit to NarrativeApp/diesel that referenced this issue Nov 26, 2020
@sgrif
Wrap all math operations in parens
18abd68 
Previously if you used local variables or parens in Rust to override the
default order of operations, we would completely lose that information
in the generated SQL. This would at best result in an unexpected value,
but at worse would cause a query like `now + 2.minutes() * (some_int +
1)` to result in a runtime error complaining that there's no operator
for `interval + integer`

I intend to backport this to 1.4

Fixes diesel-rs#2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@sgrif