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

Change error handling strategy and add verbose errors #30

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

surister
Copy link
Collaborator

@surister surister commented Jun 3, 2024

Summary of the changes / Why this is an improvement

Issue #2

This is one of the first pieces required to finish the feature and its only implemented in Python for early feedback/validation.

Main changes of PR:

  • Throwing antlr4 parse errors as exceptions is now only enabled by raise_exception API influenced by DRF Validators

  • Default behaviour is not to raise an exception, the reason is that it forces clients to try/catch errors, when you already work with CrateDB in the HTTP protocol you are used to check the json response, rather than catching exceptions, this change of behaviour aims to make integrating with this library as similar as possible as with CrateDB.

  • Implements an ExceptionCollectorListener that collects all errors that are caught, (remember that we use parser.statements, to support running several queries on the frontends, so several errors can happen simultaneously)

  • Adds more verbose errors

The main issue I would like help with the review: <-------------------------------------

  • Whenever we collect the errors I just add the errors to a list for further processing:
class ExceptionCollectorListener(ErrorListener):
    """
    Error listener that collects all errors into errors for further processing.
    Based partially on https://github.com/antlr/antlr4/issues/396
    """

    def __init__(self):
        self.errors = []

    def syntaxError(self, recognizer, offendingSymbol, line, column, msg, e):
        error = ParsingException(
            msg=msg,
            offending_token=offendingSymbol,
            e=e,
            query=e.ctx.parser.getTokenStream().getText(e.ctx.start, e.offendingToken.tokenIndex),
        )

        self.errors.append(error)

we lose the information on which statement the error belongs to, I tried with different ErrorStrategies and Error listeners but to no avail, also didn't find anything online.

My best effort is to try to match the error's offendingToken query, to any of the parsed statements:

# Shortened for clarity purposes.
def find_suitable_error(statement, errors):
    for error in errors[:]:
        if error.query == statement.query:
            statement.exception = error
            errors.pop(errors.index(error))

There are a couple of edge cases that I covered but in short, it works reasonable well, even though I'm not a fan of it, maybe you folks way more experience in antlr4 have an idea?

Checklist

  • Link to issue this PR refers to (if applicable):
  • CLA is signed

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Thank you very much. Also adding @matriv and @seut as optional reviewers, in order to a) give them a chance to spot nitty errors I am regularly missing because my reviewing style may be too lenient, and b) also give them a chance to respond to my first suggestion about API style matters.

Edit: Apologies, I missed to spot this PR is still in draft mode. So, please consider my suggestions as early feedback only.

cratedb_sqlparse_py/README.md Show resolved Hide resolved
cratedb_sqlparse_py/README.md Show resolved Hide resolved
@amotl amotl requested review from seut, matriv and amotl June 4, 2024 14:35
cratedb_sqlparse_py/README.md Show resolved Hide resolved
cratedb_sqlparse_py/README.md Show resolved Hide resolved
@surister surister marked this pull request as ready for review June 4, 2024 15:27
@surister
Copy link
Collaborator Author

surister commented Jun 4, 2024

cc @proddata I'd love your feedback as well as any input on this

@proddata
Copy link
Member

proddata commented Jun 4, 2024

I read through it and generally like the idea of not raising an exception. However, I'm not sure about the defaults.

Regarding the "main issue," I don't have a strong opinion as I lack details on the mechanics. If the approach works, though, 👍.

Copy link

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Thx for the work on this @surister !

Left a comment, but I will also checkout the branch and play around a bit, so I can understand better the code.

cratedb_sqlparse_py/tests/test_exceptions.py Show resolved Hide resolved
Copy link

@matriv matriv left a comment

Choose a reason for hiding this comment

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

thx @surister ! LGTM

@surister surister merged commit b946f76 into crate:main Jun 7, 2024
7 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.

4 participants