-
Notifications
You must be signed in to change notification settings - Fork 33
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
WIP: V1 artifact files endpoint changes #658
base: dev
Are you sure you want to change the base?
Conversation
Add initial schema and controller updates for handling browsing and retrieving artifact files.
@@ -46,6 +47,7 @@ | |||
from .service import ( | |||
RESOURCE_TYPE, | |||
SEARCHABLE_FIELDS, | |||
ArtifactIdContentsService, |
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.
causes an import error right now. I realize you haven't worked on the service layer yet, but I found it useful to create a class stub, so flask would run and I could view the endpoint and schema in swagger.
attribute="relative_path", | ||
metadata=dict(description="Relative path to the Artifact File."), | ||
) | ||
artifactType = fields.String( |
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.
will need to coordinate artifact types with @hbooth 's work. Probably should be an enum see here for an example:
https://github.com/usnistgov/dioptra/blob/a661aaa168788dffb1d7dcb1c95952b4106e9b7e/src/dioptra/restapi/v1/schemas.py#L211C1-L229C1
load_default=None, | ||
) | ||
|
||
|
||
class ArtifactGetQueryParameters( | ||
PagingQueryParametersSchema, | ||
GroupIdQueryParametersSchema, | ||
SearchQueryParametersSchema, |
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 we should leave out Search and SortBy query parameters at least for now. These would require a custom implementation since search and sort for resources are implemented via the sqlalchemy.
required=True, | ||
) | ||
files = fields.Nested( | ||
ArtifactFileSchema, |
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.
should be the paged schema.
required=True, | ||
) | ||
fileTree = fields.Nested( | ||
ArtifactFileSchema, |
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.
need a schema for the file tree view. I'm not sure if we defined this yet. I'm also not sure how it would work with the paging parameters.
The intent of the file tree was to provide the frontend with a nested file tree structure that makes it easy to display in a file browser like UI. If we are concerned with the amount of data that might be returned and need paging, maybe we can instead remove fileTree
and have the UI use files
, but add a recurse
boolean query param that determines if we want a flat list of all files, or if we just want the files in the directory in the path
query param. This way the UI would work by making a call to /artifacts/{id}/contents?path=PATH&recurse=False
, updating the path
each time the user clicks a file/directory in the browser.
) | ||
|
||
|
||
class DownloadPathQueryParametersSchema(Schema): |
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.
Consider renaming to something like ArtifactContentsQueryParameterSchema
(especially if we add additional fields like recurse
) since we are grouping a couple different parameters all related to the contents or files associated with the artifact.
|
||
|
||
class DownloadPathQueryParametersSchema(Schema): | ||
"""A schema for adding path and downlaod query parameters to a resource endpoint.""" |
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.
minor typo: downlaod -> download
No description provided.