E2e/feature/ri 6231 spike] fix the rounding issue for json data type #4089
Conversation
…nto be/feature/RI-6231-SPIKE]-Fix-the-rounding-issue-for-JSON-data-type
@romansergeenkosofteq do we have a big json anywhere is test files or docs that we can use for the second thing? |
ArtemHoruzhenko
had a problem deploying
to
e2e-approve
GitHub Actions
Error
ArtemHoruzhenko
had a problem deploying
to
e2e-approve
GitHub Actions
Error
There was a problem hiding this comment.
- I am not sure that it is a good solution to parse JSON in the slice, I would recommend set in the store what we receive from BE. The responsibility how to work with that data should be on component layer.
- Please, use our linter (we don't use semicolons) and add empty line at the end of the files
Updated the approach to not parse the jsons in the slice, but in a component directly. Added it in a util in case
@ArtemHoruzhenko I've also updated the BE tests to check the new structure. Surprisingly the DTO didn't require changes |
There was a problem hiding this comment.
@KIvanow there are some ESLint errors in the utils.ts (@typescript-eslint/indent, no-trailing-spaces, arrow-parens, no-param-reassign, import/order). Please use our eslint settings for IDE.
mariasergeenko
temporarily deployed
to
e2e-approve
GitHub Actions
Inactive
mariasergeenko
temporarily deployed
to
e2e-approve
GitHub Actions
Inactive
mariasergeenko
temporarily deployed
to
e2e-approve
GitHub Actions
Inactive
|
Testing is done :) |
| setChangedValue('null') | ||
| } else { | ||
| setChangedValue(value) | ||
| } |
There was a problem hiding this comment.
Could you please add stringify function which will return value?
There was a problem hiding this comment.
Do you mean like so -> 238acd9 having a function wrapping up this functionality, instead of doing it directly in the useEffect, or something else?
There was a problem hiding this comment.
yes, but could you please move it to utils and call smth like stringifyScalarValue?
There was a problem hiding this comment.
done, covered it with tests as well 2d08a99
I usually wait until something needs the function from a different component,before separating it in a util, but I can see the benefit of the other approach as well
| data: parseJsonData(data?.data) | ||
| })) | ||
| .catch(reject) | ||
| }) |
There was a problem hiding this comment.
I would suggest parse json in the fetchVisualisationResults
There was a problem hiding this comment.
It was initially there, but you suggested it to be on the component level, since fetchVisualisationResults is in the slice itself and the slices should simply store the data from the server in a previous comment #4089 (review)
Do you mean going back to the initial implementation or something else?
There was a problem hiding this comment.
fetchVisualisationResults do not store results. It's ok to transform result of BE and return prepared data. I think it will be better then wrap this function in the Promise. Or another way - get stringified data from this function and parse this data after call.
There was a problem hiding this comment.
I see what you mean. Updated it.
purposfully not using json-bigin library as it fails with scientific notations and probably other edge cases
seemed like a simple enough use case to implement directly
mariasergeenko
had a problem deploying
to
e2e-approve
GitHub Actions
Error
…SON-data-type' of https://github.com/RedisInsight/RedisInsight into e2e/feature/RI-6231-SPIKE]-Fix-the-rounding-issue-for-JSON-data-type
@romansergeenkosofteq done and added the linter to be triggered on save for all of my devices. Sorry for the recurring issue |
KIvanow
had a problem deploying
to
e2e-approve
GitHub Actions
Failure
mariasergeenko
deleted the
e2e/feature/RI-6231-SPIKE]-Fix-the-rounding-issue-for-JSON-data-type
branch
Tested it with bigints as numbers, bigints as strings both as the only element in the json and the same two options as part of arrays and objects.
In the added changes testes were performed with very large jsons that trigger partial retrievals of the information (kudos to @romansergeenkosofteq for pointing the different case there). For both cases we are not parsing the data on the BE before sending it back to the UI (to avoid the bigint issues). All of the parsing is done afterwards in the client. Separated the parsing function into 2 different ones
1 - parsing when the data is simple and whole (we've retrieved the entire json and simply need to parse it)
2 - parsing the partially retrieved data. In that case the data is not just a string or a JSON valid object but looks more like this:
"data": [ { "key": "_id", "path": "[0][\"_id\"]", "cardinality": 1, "type": "string", "value": "\"6735edc1021e565aca45f9fe\"" }, { "key": "index", "path": "[0][\"index\"]", "cardinality": 1, "type": "integer", "value": "1188950299261208942" },So separating the two seemed appropriate.
Tests were added for both parsing cases.