Skip to content
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

Introduce filestorage app, support for multiple file backends and store file metadata in the database #1104

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

suricactus
Copy link
Collaborator

This PR introduces a new enhanced way to store filedata. Before just dumped everything to the Object Storage. This made everything clumsy, slow and not flexible.

The plan is to quickly migrate all projects to the new storage and delete the legacy approach.

I have closed #1065, which was the same PR.

@duke-nyuki
Copy link
Collaborator

Copy link
Contributor

@gounux gounux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review focused on docker compose and environment / migrations considerations, trying to set it up on my local.
Did not have a look at the python code that much yet.

docker-app/qfieldcloud/filestorage/models.py Show resolved Hide resolved
.env.example Show resolved Hide resolved
.env.example Show resolved Hide resolved
@suricactus
Copy link
Collaborator Author

@gounux apologies for the force push, I first rebased and then realized I had to force push.

Copy link
Contributor

@gounux gounux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2nd review round, diving deeper into it.
Mainly curiosity, sometimes naive questions to understand better.
3rd one will come soon


code = "explicit_deletion_of_last_version"
message = "Explicit deletion of last file version is not allowed!"
status_code = status.HTTP_400_BAD_REQUEST
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that the HTTP_400_BAD_REQUEST return code is used several times. Naive question : why ? Why no custom HTTP 4XX codes ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I think custom HTTP codes are not great practice. Some HTTP clients might not work great with unknown statuses and those provided by default are already good enough. We have the custom information in the code property which the client can interpret.

docker-app/qfieldcloud/core/fields.py Outdated Show resolved Hide resolved
Comment on lines +30 to +52
migrations.AddField(
model_name="project",
name="file_storage",
field=models.CharField(
help_text="Which file storage provider should be used for the storing the project related files.",
max_length=100,
validators=[qfieldcloud.core.validators.file_storage_name_validator],
verbose_name="File storage",
default=get_file_storage_name,
),
preserve_default=False,
),
migrations.AlterField(
model_name="project",
name="file_storage",
field=models.CharField(
help_text="Which file storage provider should be used for the storing the project related files.",
max_length=100,
validators=[qfieldcloud.core.validators.file_storage_name_validator],
verbose_name="File storage",
default=qfieldcloud.core.models.get_project_file_storage_default,
),
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding the field then altering it ? Wouldn't it be possible to merge the two migration "commands" into a single one ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, no, it is because of the default value.

We first create a temporary default value, that populates the current "legacy storage" name. Note that the preserve_default=False, which means that there will be no default value permanently set.
Then we set the default to be qfieldcloud.core.models.get_project_file_storage_default, which is permanent.

docker-app/qfieldcloud/core/views/package_views.py Outdated Show resolved Hide resolved
docker-app/qfieldcloud/core/views/package_views.py Outdated Show resolved Hide resolved
docker-app/qfieldcloud/core/models.py Show resolved Hide resolved
docker-app/qfieldcloud/filestorage/models.py Show resolved Hide resolved
docker-app/qfieldcloud/filestorage/models.py Outdated Show resolved Hide resolved
docker-app/qfieldcloud/filestorage/models.py Show resolved Hide resolved
docker-app/qfieldcloud/filestorage/models.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants