-
Notifications
You must be signed in to change notification settings - Fork 45
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
sanitization rules aren't applied if a custom exec is used #146
Comments
Hmm, good point. We used to use I think a good approach would be for us to keep it that way and add docs to make it clear that's how it's supposed to work. We can also add docs for a workaround. I like that you've come up with a workaround, but it does appear to use internal implementation details. A different workaround that I think would be more sustainable long term would be nested use of the library. For example, someone could specify an Alternatively, we could expand the library so that the type of Curious what your thoughts are on these paths. |
Matt,
I see your logic, but that would mean that I'd need yet another schema -
one to sanitize (custom), one to sanitize with the rules and one for
validation...
..although I guess I could call sanitize a second time on the same
schema without the custom param (although I'm guessing that would fail).
On the other hand - in the current exec, you a can call this.report()
and so allowing this.rules(...) wouldn't be entirely breaking with
convention?
- and...when documented, it wouldn't be internal implementation
details any more :-)
Regards,
Steve
p.s. Thanks for the quick reply!
…------ Original Message ------
From: "Matt Welke" ***@***.***>
To: "schema-inspector/schema-inspector"
***@***.***>
Cc: "steve-allam" ***@***.***>; "Author"
***@***.***>
Sent: 30/03/2023 16:41:06
Subject: Re: [schema-inspector/schema-inspector] sanitization rules
aren't applied if a custom exec is used (Issue #146)
Hmm, good point. We used to use exec at a previous company for
validation, but we didn't have any other rules in place besides it. So
that explains why I never ran into this before. When I look at the
README, it doesn't make it clear whether exec is meant to add to the
rules specified or replace any other rules specified. My gut feeling is
that the original library author meant for it to be a kind of escape
hatch where it would completely override any other rules specified.
I think a good approach would be for us to keep it that way and add
docs to make it clear that's how it's supposed to work. We can also add
docs for a workaround. I like that you've come up with a workaround,
but it does appear to use internal implementation details.
Something I think would be more sustainable long term would be nested
use of the library. For example, someone could specify an exec function
that did their custom behavior and then used another instance of Schema
Inspector within the function body, with another, much smaller schema,
to apply a few more Schema Inspector rules. Or, they could do it the
other way around. First, the rules, then their custom logic. That
workaround would not involve using internal implementation details. It
would be 100% public API use.
Curious what your thoughts are on this path.
—
Reply to this email directly, view it on GitHub
<#146 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APGIZA5DZRUMY67TO2N2OTDW6WSRFANCNFSM6AAAAAAWNAJM6I>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Yeah thinking about this again, my workaround does sound pretty annoying. It sucks to have to use more than one schema when you've done the work to write a schema that can be used for both validation and sanitization, which is literally this library's reason to exist in a world where we have JSON Schema already. I think my idea for adding support for "additive exec" is the way to go. Because this library is in maintenance mode right now (I'm the only maintainer and I'm not paid to work on OSS), I wasn't originally planning to add new features, just bug fixes and security vulnerabilities. To me, this feature lacking sounds like a bug. So I'll complete the work when I can. I can't make any promises on timeline though. I recommend implementing the multiple schema workaround I suggested if you need to get around this bug urgently. If you're in no rush, feel free to keep an eye on the releases for the fix. |
Matt,
That's fine - I'll leave my "internal knowledge" fix in place for now,
as its not used in a life and death application, so if it breaks at any
point I can easily fix.
- A project that I'm not paid for as well :-)
Regards,
Steve
…------ Original Message ------
From: "Matt Welke" ***@***.***>
To: "schema-inspector/schema-inspector"
***@***.***>
Cc: "steve-allam" ***@***.***>; "Author"
***@***.***>
Sent: 31/03/2023 18:04:07
Subject: Re: [schema-inspector/schema-inspector] sanitization rules
aren't applied if a custom exec is used (Issue #146)
Yeah thinking about this again, my workaround does sound pretty
annoying. It sucks to have to use more than one schema when you've done
the work to write a schema that can be used for both validation and
sanitization, which is literally this library's reason to exist in a
world where we have JSON Schema already.
I think my idea for adding support for "additive exec" is the way to
go.
Because this library is in maintenance mode right now (I'm the only
maintainer and I'm not paid to work on OSS), I wasn't originally
planning to add new features, just bug fixes and security
vulnerabilities. To me, this feature lacking sounds like a bug. So I'll
complete the work when I can. I can't make any promises on timeline
though. I recommend implementing the multiple schema workaround I
suggested if you need to get around this bug urgently. If you're in no
rush, feel free to keep an eye on the releases for the fix.
—
Reply to this email directly, view it on GitHub
<#146 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APGIZA3OQVPFIZVYAKRYLTLW64FAPANCNFSM6AAAAAAWNAJM6I>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Not sure if this is expected behaviour, as the documentation doesn't say either way (that I can see).
I have pasted an example script below. I am using a custom function 'normalise' to match various options and convert to my required string, but it would appear that when using this - the rules aren't applied. ['trim','lower'] doesn't happen. If you run the script as below, the gender will come out as 'Male', whereas I would expect it to be 'male'.
I have added a line in the normalise function to apply the rules, but I expected them to be applied anyway?
Regards,
Steve
p.s. Many thanks for a useful library!
The text was updated successfully, but these errors were encountered: