-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(raw-api): add an endpoint to retrieve files in their original format #2244
feat(raw-api): add an endpoint to retrieve files in their original format #2244
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One test you wrote seems to fail on Windows
from antarest.study.storage.rawstudy.model.helpers import FileStudyHelpers | ||
from antarest.study.storage.utils import extract_output_name, fix_study_root, remove_from_cache | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
def _assert_not_folder_node(file_node: INode[t.Any, t.Any, t.Any]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do this inside the get_file method before calling the extract_xxx to simplify the code.
Also I don't really like the name of this exception, it could be better to rename it something like:
PathIsAFolderError with a msg like f"The given path {file_node.config.path} points to a folder and not a file"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically what I was going to do but the functions extract_xxx
should only accept a file node
, and there is no class that abstracts this concept.
So In the end you should raise a folder Error
in each extract_xxx
or create a type that excludes non-file-nodes
.
The first option is better.
content=read_original_file_in_archive(archive_path, relative_path_inside_archive), | ||
filename=file_node.config.path.name, | ||
) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here it makes the code less readable I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, however a comment about "links" handling which should not go into the service layer.
eba6d98
to
3ec6074
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems nice just a few comments
antarest/study/storage/rawstudy/model/filesystem/folder_node.py
Outdated
Show resolved
Hide resolved
|
||
# Then, use the API to download the files from the "user/folder" directory | ||
user_folder_dir = study_dir.joinpath("user/folder") | ||
for file_path in user_folder_dir.glob("*.*"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see the point of this part, you can remove it IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is the API test, so you retrieve a file via the API and verify that the bytes content match with what you have on disk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you do this for many files so you know there is no a specific extension that does not work
assert "Node is a folder node." in res.json()["description"] | ||
assert res.json()["exception"] == "PathIsAFolderError" | ||
|
||
def test_retrieve_original_file_from_archive(self, client: TestClient, user_access_token: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be better to parametrize this test, archived and unarchived.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the unarchived case it is done in the previous review and I would like to follow the same logic as we had with raw
API before:
- tests for unarchived study with many file types
- specific test for an archived study
) | ||
res.raise_for_status() | ||
|
||
# skip the test if the OS is Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't skip this test on windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we already talked about this and agreed to ignore it for Windows as it fails for a link in an archive.
9232dac
to
72ae4fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few implementation details
antarest/study/storage/rawstudy/model/filesystem/matrix/input_series_matrix.py
Outdated
Show resolved
Hide resolved
antarest/study/storage/rawstudy/model/filesystem/matrix/input_series_matrix.py
Outdated
Show resolved
Hide resolved
antarest/study/storage/rawstudy/model/filesystem/matrix/input_series_matrix.py
Outdated
Show resolved
Hide resolved
…nk` only in `inode`
8b89305
to
8f9a4ad
Compare
Context:
Users found it a little inconvenient to work with some files (retrieved using the
raw
API) e.g. dealing with a.json
file while it should be a.ini
file as it is in its original formatSolution:
Create an new endpoint to retrieve files in their original format