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

Image corruption when editing other fields #799

Open
2 tasks done
mrpavelgoodman opened this issue Aug 12, 2024 · 2 comments
Open
2 tasks done

Image corruption when editing other fields #799

mrpavelgoodman opened this issue Aug 12, 2024 · 2 comments

Comments

@mrpavelgoodman
Copy link

Checklist

  • The bug is reproducible against the latest release or master.
  • There are no similar issues or pull requests to fix it yet.

Describe the bug

If you try to edit any fields of the form or submit the form unchanged, the image file that was uploaded earlier will be corrupted.

image

Here is my code:

from fastapi_storages import FileSystemStorage

storage = FileSystemStorage(path="static/images/")

class Subcategory(IntIdPkMixin, Base, Creatable, Updatable, SoftDeletable):
    name_en: Mapped[str] = mapped_column(String(50), nullable=False)
    name_ru: Mapped[Optional[str]] = mapped_column(String(50), nullable=True)
    tag_en: Mapped[Optional[str]] = mapped_column(String(50), nullable=True)
    tag_ru: Mapped[Optional[str]] = mapped_column(String(50), nullable=True)
    image_url: Mapped[Optional[str]] = mapped_column(
        FileType(storage=storage), nullable=True
    )
    is_main: Mapped[bool] = mapped_column(Boolean, default=False, nullable=False)
    slug: Mapped[Optional[str]] = mapped_column(String(100), nullable=True, unique=True)
    category_id: Mapped[int] = mapped_column(ForeignKey("categorys.id"))
    category: Mapped["Category"] = relationship(
        "Category", back_populates="subcategories"
    )
    products: Mapped[List["Product"]] = relationship(
        "Product", back_populates="subcategory"
    )

    def __str__(self):
        return self.name_ru

class SubcategoryAdmin(ModelView, model=Subcategory):
    name = "Подкатегории"
    name_plural = "Подкатегории"
    icon = "fa-solid fa-tags"
    category = "Товары"
    edit_template = "sqladmin/edit.html"

    column_list = [
        Subcategory.id,
        Subcategory.slug,
        Subcategory.name_en,
        Subcategory.name_ru,
        Subcategory.tag_en,
        Subcategory.tag_ru,
        Subcategory.is_main,
        "category.name_en",
        "category.name_ru",
    ]
    column_searchable_list = [Subcategory.name_en, Subcategory.name_ru]
    column_sortable_list = [
        Subcategory.id,
        Subcategory.slug,
        Subcategory.name_en,
        Subcategory.name_ru,
        Subcategory.tag_en,
        Subcategory.tag_ru,
        Subcategory.is_main,
        "category.name_en",
        "category.name_ru",
    ]
    can_create = True
    can_edit = True
    can_delete = True
    can_view_details = True

    form_columns = [
        "category",
        "name_en",
        "name_ru",
        "tag_en",
        "tag_ru",
        "image_url",
        "is_main",
    ]

    column_formatters = {
        "category.name_en": lambda m, a: m.category.name_en if m.category else "N/A",
        "category.name_ru": lambda m, a: m.category.name_ru if m.category else "N/A",
    }

    column_labels = {
        "id": "ID",
        "name_en": "Название подкатегории (англ.)",
        "name_ru": "Название подкатегории (рус.)",
        "tag_en": "Тег (англ.)",
        "tag_ru": "Тег (рус.)",
        "image_url": "URL изображения",
        "is_main": "Главная",
        "slug": "Слаг",
        "category_id": "ID Категории",
        "created_at": "Дата создания",
        "updated_at": "Дата обновления",
        "category.name_en": "Подкатегория (англ.)",
        "category.name_ru": "Подкатегория (рус.)",
        "products": "Продукты",
        "category": "Категория",
    }

    async def on_model_change(self, data, model, is_created, request):

        form = await request.form()

        for key, value in form.items():
            app_logger.info(
                f"Field name: {key}, Field type: {type(value)}, Field value: {value}"
            )

        if "image_url" in form:
            image = form["image_url"]

            if image.filename == "":
                data.pop("image_url", None)
                app_logger.info("Image field drop")

        model.slug = generate_slug(model.name_en)
        app_logger.info(data)
        async with Session(expire_on_commit=False) as session:
            subcategory_service = SubcategoryService(session)
            await subcategory_service.set_main_subcategory(model.id)

Steps to reproduce the bug

1.Go to the edit page
2. Save the edits without uploading a new image.
4. If you check the image that was there earlier, it will be damaged.

Expected behavior

it is expected to receive modified data in other fields, without damaging the image

Actual behavior

No response

Debugging material

I did a little research. I found that when submitting the form, if you do not embed a new image, there is file data in the form, but they are empty.
web_1 | INFO 2024-08-12 07:05:52.875 request id: None - core.admin:on_model_change - Field name: image_url, Field type: <class 'starlette.datastructures.UploadFile'>, Field value: UploadFile(filename='', size=0, headers=Headers({'content-disposition': 'form-data; name="image_url"; filename=""', 'content-type': 'application/octet-stream'}))

By adding a simple check and removing this field from the data, I avoid the problem of file corruption:
` async def on_model_change(self, data, model, is_created, request):

    form = await request.form()

    for key, value in form.items():
        app_logger.info(
            f"Field name: {key}, Field type: {type(value)}, Field value: {value}"
        )

    if "image_url" in form:
        image = form["image_url"]

        if image.filename == "":
            data.pop("image_url", None)
            app_logger.info("Image field drop")`

Environment

ubuntu 23.04, python 3.10, sqladmin[full] latest, fastapi 0.111, sqlalchemy[asyncio]~=2.0.30

Additional context

No response

@AntoshkaTeck
Copy link

I encountered the exact same problem and solved it in the following way.
I created a custom type FileType and overrode the process_bind_param function. I added transliteration of the file name from Cyrillic (otherwise, the name gets erased, leaving only the extension, for example: название_файла.png -> png).
Additionally, as mentioned above, I found a bug with file corruption and added a check.

from fastapi_storages.integrations.sqlalchemy import FileType as _FileType

class FileType(_FileType):
    def process_bind_param(self, value: Any, dialect: Dialect) -> Optional[str]:
        if value is None:
            return value
        if len(value.file.read(1)) != 1:
            return None

        #Transliteration

        file_name_language = detect_language(value.filename)
        file_name = translit(value.filename, reversed=True) if file_name_language else value.filename

        file = StorageFile(name=file_name, storage=self.storage)
       
       #Fix file corruption
        if value.size is None:
            return file.name

        file.write(file=value.file)

        value.file.close()
        return file.name

@aminalaee
Copy link
Owner

Hey, thanks both for the investigations.
I think it was already discussed somewhere else, but in this case your file field was nullable so you could do this.
We also have to consider the case where the file field is required and we are editing this.

Looking at Django admin seems like they work around this that if the file field is required but are editing an object that already has a file, then the field is optional so you either override it or the existing one remains. I think we can follow the same approach we need changes both in SQLAdmin and fastapi-storages. I will look into this.

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

No branches or pull requests

3 participants