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

Return errors, not panic, when integers fail to parse in AUTO_INCREMENT and TOP #1305

Merged
merged 4 commits into from
Jun 18, 2024

Conversation

eejbyfeldt
Copy link
Contributor

@eejbyfeldt eejbyfeldt commented Jun 10, 2024

This fixes several places where the code currently panic when parsing an integer value. This change makes us return the error instead.

This replaces #1304 by also handling one more cases and introducing a helper method that can be used in all places when an integer value needs to be parsed.

Closes #1303

@eejbyfeldt eejbyfeldt changed the title Return errors when integers fail to parse Return errors when integers fail to parse in AUTO_INCREMENT and TOP Jun 10, 2024
Copy link
Contributor

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is a big problem, especially for people like me who use sqlparser inside a web server. @alamb, can we prioritize merging this ?

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- thank you @eejbyfeldt and @lovasoa

@alamb alamb changed the title Return errors when integers fail to parse in AUTO_INCREMENT and TOP Return errors, not panic, when integers fail to parse in AUTO_INCREMENT and TOP Jun 18, 2024
@alamb alamb changed the title Return errors, not panic, when integers fail to parse in AUTO_INCREMENT and TOP Return errors, not panic, when integers fail to parse in AUTO_INCREMENT and TOP Jun 18, 2024
@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9454797383

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 23 of 24 (95.83%) changed or added relevant lines in 2 files are covered.
  • 12 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.09%) to 88.968%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/sqlparser_common.rs 8 9 88.89%
Files with Coverage Reduction New Missed Lines %
src/lib.rs 1 12.64%
src/parser/alter.rs 2 66.67%
tests/sqlparser_common.rs 4 89.57%
src/dialect/mod.rs 5 72.08%
Totals Coverage Status
Change from base Build 9439708625: 0.09%
Covered Lines: 26144
Relevant Lines: 29386

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented Jun 18, 2024

@eejbyfeldt
Copy link
Contributor Author

eejbyfeldt commented Jun 18, 2024

There appears to be a CI failure on this PR: https://github.com/sqlparser-rs/sqlparser-rs/actions/runs/9454797383/job/26361666986?pr=1305

Fixed the issue. And enabled CI on my fork to catch these kind of issues earlier next time.

@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9565500013

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 2 files are covered.
  • 610 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.09%) to 88.972%

Files with Coverage Reduction New Missed Lines %
src/lib.rs 1 12.64%
src/parser/alter.rs 2 66.67%
src/dialect/mod.rs 5 72.08%
src/ast/dml.rs 42 73.47%
src/parser/mod.rs 112 93.2%
src/ast/mod.rs 192 81.5%
tests/sqlparser_common.rs 256 89.58%
Totals Coverage Status
Change from base Build 9439708625: 0.09%
Covered Lines: 26141
Relevant Lines: 29381

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented Jun 18, 2024

Thanks again @eejbyfeldt

@alamb alamb merged commit 79af31b into apache:main Jun 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return error when AUTO_INCREMENT offset is too large
4 participants