feat(output): implement a retrocompatibility layer for /raw endpoint#3093
feat(output): implement a retrocompatibility layer for /raw endpoint#3093TheoPascoli wants to merge 17 commits intodevfrom
Conversation
sylvlecl
left a comment
sylvlecl
left a comment
There was a problem hiding this comment.
Looks pretty good already ! but a few things can be made simpler or moved
antarest/output/output_service.py
Outdated
| frequency: MatrixFrequency, | ||
| columns_names: Sequence[str], | ||
| ids_to_consider: Sequence[str], | ||
| transform_columns_headers: bool = True, |
There was a problem hiding this comment.
should be remove for this endpoint too
There was a problem hiding this comment.
Kept: both call sites need different values: False for the legacy path (splittable column names), True for the new matrix format. Tell me if you have any idea
antarest/output/output_service.py
Outdated
|
|
||
| return download_id | ||
|
|
||
| def aggregate_output_data( |
There was a problem hiding this comment.
I think this one can be removed now
antarest/study/service.py
Outdated
| frequency=parsed.frequency, | ||
| item_id=parsed.ids_to_consider, | ||
| mc_year=parsed.mc_year, | ||
| transform_columns_headers=False, |
There was a problem hiding this comment.
don't think we need the False here, it's only meant to be used by the "download" endpoint which will not go through that method --> hence I think we can remove the parameter also from the methode
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14433192 | Triggered | Authentication Tuple | b284fa2 | antarest/dependencies.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
sylvlecl
left a comment
sylvlecl
left a comment
There was a problem hiding this comment.
I think there is still one thing to fix, so that formatting options are correctly taken into account
antarest/study/service.py
Outdated
| item_id=parsed.ids_to_consider, | ||
| mc_year=parsed.mc_year, | ||
| ) | ||
| return { |
There was a problem hiding this comment.
We should just return the polars dataframe here so that it is then formatted by the web layer.
See that piece of code:
if isinstance(output, pl.DataFrame):
if matrix_format is None:
matrix_format = MatrixFormat.JSON if formatted else MatrixFormat.PLAIN
return matrix_format.serialize_dataframe(output)Makes me think that we probably don't have integration tests that cover this correctly:
I think in the current state, the "formatting" options (formatted & matrix_format) of the endpoint will not be taken into account correctly, since they are used by the web layer only if this returns a dataframe.
--> we should add an integration test which checks the behaviour of those parameters for an output matrix (we can use the STA-mini study for that)
2 participants
No description provided.