[CORE-325] Get Filters#188
Conversation
| for _, raw := range answers { | ||
| var data map[string]any | ||
| err = json.Unmarshal(raw, &data) | ||
| if err != nil { |
There was a problem hiding this comment.
The unmarshal error is silently ignored. Please log a warning so that malformed data in the database can be traced.
There was a problem hiding this comment.
Sorry for the misleading comment — this should actually be logged as error rather than logged as warn. A failed unmarshal here means the data already stored in the database is malformed, which is an unexpected state that should be surfaced clearly.
| seen := make(map[uuid.UUID]bool) | ||
|
|
||
| for _, raw := range answers { | ||
| var data map[string]any |
There was a problem hiding this comment.
Unmarshal into the existing structs (e.g. SingleChoiceAnswer for single-choice, MultipleChoiceAnswer / DetailedMultipleChoiceAnswer for multi-choice) instead of using a raw map[string]any. This makes the code more readable and safer.
| Files []shared.UploadFileEntry `json:"files"` | ||
| } | ||
|
|
||
| type FilterAnswerRequest struct { |
There was a problem hiding this comment.
The outer filters wrapper in FilterAnswerRequest seems unnecessary since this endpoint already targets a specific questionId via the path. Consider flattening the request body to just { "answers": [{ "option": "uuid" }] }.
There was a problem hiding this comment.
FilterAnswerRequest records the answers that had already been chosen, so there might be multiple questionId and their answers. ( And the questionId might not be the one in the path ) That's why I use the outer filters to wrap the struct.
There was a problem hiding this comment.
Thanks for the explanation. On the other hand, Answers []struct currently only contains Option, but if there's a chance more fields will be added in the future, consider extracting it into a named struct for readability and easier extension.
There was a problem hiding this comment.
According to the previous discussion, I think we will named it Option for now... since every column representing matchValue is named Option
| defer span.End() | ||
| logger := logutil.WithContext(traceCtx, h.logger) | ||
|
|
||
| formIdStr := r.PathValue("formId") |
There was a problem hiding this comment.
formId and viewId are parsed from the path but not used in any business logic — they are only echoed back in the response. Consider whether they should be validated), or if parsing them is unnecessary.
There was a problem hiding this comment.
I remember that we discussed in the meeting and decided to parse formId and viewId, but actually I don't remember why...QQ
Should I bring this out in the next meeting?
There was a problem hiding this comment.
I think this might have been included under the assumption that it would be used. We may discuss this further in the next meeting.
3 participants
Type of changes
Purpose
Additional Information