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

Dump the rule for ACL formula warnings #639

Merged
merged 5 commits into from
Aug 23, 2023

Conversation

fflorent
Copy link
Collaborator

@fflorent fflorent commented Aug 22, 2023

When an ACL formula fails to be run, a warning is printed. However, it is painful to know which formula is concerned by the warning.

It is especially painful when this error is printed in production logs.

This fix prints the formula in the warning.

Also (and that may be controversial), I print the whole stack trace instead of just the error message. This helps to understand which keywork is concerned by the error. But in the other hand, it may spam the logs. I would understand if you ask me to rollback this part.

The log how it used to be

ACLRule for my_table failed: Cannot read property 'includes' of null

The log as it will be if accepted

warn: ACLRule for my_table (`rec.some_property in user.custom.column_which_may_be_null`) failed: TypeError: Cannot read properties of null (reading 'includes')
    at /home/florent/projects/grist-core/_build/app/server/lib/ACLFormula.js:48:66
    at /home/florent/projects/grist-core/_build/app/server/lib/ACLFormula.js:98:23
    ...

@fflorent fflorent requested a review from paulfitz August 22, 2023 13:16
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

As an incremental change, I think I'm ok with this.

Fundamentally logging rule failures like this isn't as helpful as it could be, ideally someone related to the document itself could be getting notified. In general a user won't have access to server logs, or know to ask someone to look at them.

Separately, the example error you gave, with ".includes", is also the one I've seen lead to errors. It seems innocuous, and a pain to have to write "X and Y in X" rather than just "Y in X". What if we decided to define "Y in null" and as "false" for access rules? We can, since access rules are not actually evaluated by python, and the error message is already different to the error you'd get in python.

@fflorent
Copy link
Collaborator Author

@paulfitz Something like my latest commit?

I don't know where I can write test for this piece of code. I may take a look tomorrow if you think it is worth.

@fflorent fflorent requested a review from paulfitz August 22, 2023 17:43
@paulfitz
Copy link
Member

@fflorent yes, that kind of change is what I had in mind.

I found and liberated a test file that may be relevant f6e8b9c

When an ACL formula fails to be run, a warning is printed. However, it
is painful to know which formula is concerned by the warning.
@fflorent
Copy link
Collaborator Author

Thank you! I added the tests.

Also I took the liberty to made some changes to your tests, in order to split it into several smalls one, with the hope its fine like this.

@fflorent
Copy link
Collaborator Author

I also saw that if the RHS of in / not in operators is a string, it should check whether the LHS is a substring of the RHS:

>>> 'foo' in 'foobar'
True
>>> 'foo' in 'foo'
True
>>> 'foo' in 'fo'
False
>>> 'foo' not in 'fo'
True
>>> 'foo' not in 'foo'
False

The ACL formula engine works as in Python, I guess that's fine.

I added some tests to check this behavior:
90e547f

Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Looking good, a few suggestions.

app/server/lib/ACLFormula.ts Outdated Show resolved Hide resolved
app/server/lib/ACLFormula.ts Outdated Show resolved Hide resolved
app/server/lib/PermissionInfo.ts Outdated Show resolved Hide resolved
test/server/lib/ACLFormula.ts Show resolved Hide resolved
test/server/lib/ACLFormula.ts Show resolved Hide resolved
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Thanks @fflorent! Test failures look unrelated - flaky tests that probably need some more careful wait conditions somewhere, but not your responsibility.

@fflorent
Copy link
Collaborator Author

Great! Also thanks for your suggestion here, that will be much more convenient for cases when an array is expected but is nullish (like an empty cell).

@paulfitz paulfitz merged commit 9dfebef into gristlabs:main Aug 23, 2023
12 of 13 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.

2 participants