Validate against forbidden fields #232
Conversation
wejdross
requested review from
a team,
Kidswiss,
TheBigLee and
zugao
and removed request for
a team
| } | ||
|
|
||
| for key, _ := range pgConf { | ||
| if key == "listen_addresses" { |
There was a problem hiding this comment.
Can you please define a list of forbidden pg parameters on top of this file and instead of having multiple if clauses to just have one? I would also consider create a new go file to separate the concerns and make it a bit more readable
There was a problem hiding this comment.
I agree, having a map of forbidden keys would be more readable. You can use the maps key for the field name, so you can efficiently do checks. Then use the value of the map to store the specifics of the error.
It would also be easier to extend or change in the future.
There was a problem hiding this comment.
The errors seems to be almost the same, only the name is added to a field. I would still go with just a list.
| } | ||
|
|
||
| for key, _ := range pgConf { | ||
| if key == "listen_addresses" { |
There was a problem hiding this comment.
I agree, having a map of forbidden keys would be more readable. You can use the maps key for the field name, so you can efficiently do checks. Then use the value of the map to store the specifics of the error.
It would also be easier to extend or change in the future.
| } | ||
| } | ||
|
|
||
| blocklist := map[string]string{ |
There was a problem hiding this comment.
why did you create a map if the values are empty? Please change to a list and move it outside the function at the top of the file. Our code is already very confusing, lets not make it a pain to read.
There was a problem hiding this comment.
The idea for the map is that we don't have nested loops, so I would keep it as such.
But I agree to move it to the top.
There was a problem hiding this comment.
You meant that there's no good way to search in the list? I forgot how bad go is in that regard... but there are packages that help with finding an object in the list... I would still have gone with the list... it hearts my eyes to see empty strings there, it feels like they will be filled during a for loop....
There was a problem hiding this comment.
finding an element in array is the same operation in each language, finding key in map is O(1) . O(n^2) You suggest would hurt my eyes ;p
Summary
Checklist
bug,enhancement,documentation,change,breaking,dependencyas they show up in the changelog