feat: validate username and db_name format #366
Conversation
|
cc @hauleth |
lib/supavisor/client_handler.ex
Outdated
|
|
||
| if String.contains?(user, not_allowed) or String.contains?(db_name, not_allowed) do | ||
| reason = "Invalid characters in user or db_name" | ||
| if !String.match?(user, rule) or !String.match?(db_name, rule) do |
There was a problem hiding this comment.
I prefer using not over !. Also I think that =~ would be cleaner there.
There was a problem hiding this comment.
Also I think that =~ would be cleaner there.
good call!
There was a problem hiding this comment.
I prefer using not over !
is it just because of readability or is there some other benefit?
There was a problem hiding this comment.
2 things:
- I find it more readable
!works on any type,notworks only on booleans
Technically not could be faster (though I doubt that it would be noticeable) and it also will warn you faster if there will be something off.
There was a problem hiding this comment.
Name ips average deviation median 99th %
! 81.42 M 12.28 ns ±1101.51% 12.08 ns 13.75 ns
not 78.34 M 12.77 ns ±1097.19% 12.50 ns 14.58 ns
Comparison:
! 81.42 M
not 78.34 M - 1.04x slower +0.48 ns
Benchee.run(%{
"!" => fn ->
!true
end,
"not" => fn ->
not true
end
})There was a problem hiding this comment.
So unnoticeable difference. Also I do not see how ! can be faster, and there I think it could be optimised away by the compiler anyway.
lib/supavisor/client_handler.ex
Outdated
|
|
||
| if String.contains?(user, not_allowed) or String.contains?(db_name, not_allowed) do | ||
| reason = "Invalid characters in user or db_name" | ||
| if !(user =~ rule && db_name =~ rule) do do |
There was a problem hiding this comment.
I preferred it in older form
| if !(user =~ rule && db_name =~ rule) do do | |
| if not user =~ rule or not db_name =~ rule do |
There was a problem hiding this comment.
Alternatively we can swap bodies of if and use positive predicate there.
lib/supavisor/client_handler.ex
Outdated
|
|
||
| if String.contains?(user, not_allowed) or String.contains?(db_name, not_allowed) do | ||
| reason = "Invalid characters in user or db_name" | ||
| if !(user =~ rule && db_name =~ rule) do |
There was a problem hiding this comment.
I preferred older form of:
| if !(user =~ rule && db_name =~ rule) do | |
| if not user =~ rule or not db_name =~ rule do |
Though I would probably swap bodies of the if and use positive predicate there, so it would be:
if user =~ rule and db_name =~ rule do
# old `else` block content
else
# old `if` block content
endThere was a problem hiding this comment.
100%, just need to swap cases 😄
No description provided.