Feat/kafka mapping support #420
Conversation
| $row[$fieldName] = $this->morphValuesByFieldType($message[$fieldName], $fieldType); | ||
| $inputKeyName = $fieldName; | ||
| if ($this->customMapping !== [] | ||
| && isset($this->customMapping[$fieldName])) { |
There was a problem hiding this comment.
isset should be enough?
There was a problem hiding this comment.
In case isset is enough let's remove $this->customMapping !== [] cuz it does not do anything, isset is enough
There was a problem hiding this comment.
No, because during the construct, we use json_decode. If the JSON is corrupted, it can return false. That’s why we check if it’s an array.
There was a problem hiding this comment.
You have declaration
private array $customMapping;
In case it returns false and we try to assign, we will fail already before this code, so I guess [] can be removed
$decodedMapping = json_decode($instance['custom_mapping'], true);
$this->customMapping = $decodedMapping;What if we will do this thing:
$decodedMapping = json_decode($instance['custom_mapping'], true) ?: []So it will make sure we have array even if it returns false and avoid strict type fatal error
| @@ -104,6 +104,7 @@ public function runPool(): void { | |||
| * buffer_table:string, | |||
| * destination_name:string, | |||
| * query:string, | |||
| * custom_mapping: string, | |||
There was a problem hiding this comment.
We request string here but in parseMapping method returns false in case json failed to encode, is it normal?
There was a problem hiding this comment.
I have never encountered issues with json_encode. In my opinion, it’s nearly impossible to encode simple array with an error
There was a problem hiding this comment.
JSON easy to fail in case we accept user data. Does the encoded array get information from the outside of the world? User provide it? in case easy to trigger false so with unsupported Unicodes
There was a problem hiding this comment.
I agree that adding some additional checks to the mapping provided by the user is a good idea. However, after these checks, we should assume the data is correct and that there is no possibility of encoding/decoding errors.
| $row[$fieldName] = $this->morphValuesByFieldType($message[$fieldName], $fieldType); | ||
| $inputKeyName = $fieldName; | ||
| if ($this->customMapping !== [] | ||
| && isset($this->customMapping[$fieldName])) { |
There was a problem hiding this comment.
In case isset is enough let's remove $this->customMapping !== [] cuz it does not do anything, isset is enough
| @@ -104,6 +104,7 @@ public function runPool(): void { | |||
| * buffer_table:string, | |||
| * destination_name:string, | |||
| * query:string, | |||
| * custom_mapping: string, | |||
There was a problem hiding this comment.
JSON easy to fail in case we accept user data. Does the encoded array get information from the outside of the world? User provide it? in case easy to trigger false so with unsupported Unicodes
There was a problem hiding this comment.
Looks good, but still have one issue with strict type checking when json decode returns false and we try to assign it to property declared as array, also we are hiding this checker with annotation, that probably not very good idea, cuz phpstan shows this issue
| $row[$fieldName] = $this->morphValuesByFieldType($message[$fieldName], $fieldType); | ||
| $inputKeyName = $fieldName; | ||
| if ($this->customMapping !== [] | ||
| && isset($this->customMapping[$fieldName])) { |
There was a problem hiding this comment.
You have declaration
private array $customMapping;
In case it returns false and we try to assign, we will fail already before this code, so I guess [] can be removed
$decodedMapping = json_decode($instance['custom_mapping'], true);
$this->customMapping = $decodedMapping;What if we will do this thing:
$decodedMapping = json_decode($instance['custom_mapping'], true) ?: []So it will make sure we have array even if it returns false and avoid strict type fatal error
No description provided.