Fix s3 cloud compatibility on Local Windows

[samialfattani]

• this change needed whever the current os platform is Windows. In this case the os.path.pathnorm() will not work properly since it treats storage as it is placed in the current system.

• without this change, browsing deep path in FileAdmin view such as /admin/fileadmins3/b/xx/yy/zz will response 404 code.

[ElLorans]

Thanks! I still think you don't need on_windows param in this PR. Also, I think your test was already passing the CI CD since it runs on Linux. If that' the case, I would specify it in the test's docstrings. I can help validate that if you only rock Windows on your machine.

[samialfattani]

Thanks! I still think you don't need on_windows param in this PR. Also, I think your test was already passing the CI CD since it runs on Linux.

Yes you are right if it runs on Linux then no need for this PR at all 🥇

But let me clarify this again why the on_windows IS needed:
Suppose we have 2 FileAdmin Views (one for local directory C:\x\y and the other browsing s3 storage ) running flask-admin on Windows, in this case the platform.system() == "Windows" in BaseFileAdmin will return True for both views which is not what we expect !!
the BaseFileAdmin should explicitly know wither this storage of the current view is placed on Windows or Linux.

Another reason, for storages other than S3 such as Azure storage, it is capable to be run on both Windows and Linux. i.e the host could be on Linux while the storage is on Windows!! In this case, the AzureFileAdmin must explicitly specify where the storage is placed on. which means it has to pass on_windows parameter to BaseFileAdmin.

---

If that' the case, I would specify it in the test's docstrings. I can help validate that if you only rock Windows on your machine.

I don't know what do you mean by docstrings i might need your clarification.

[ElLorans]

Another reason, for storages other than S3 such as Azure storage, it is capable to be run on both Windows and Linux. i.e the host could be on Linux while the storage is on Windows!!

I think this replies to most of my questions! Is there a way to set up Azurite for this scenario and add that test?

[samialfattani]

I think this replies to most of my questions! Is there a way to set up Azurite for this scenario and add that test?

I am working on this, but the current test is not using Azurite, let me see if i can change it to Azurite just like the examples/azure

[samuelhwilliams]

Hi 👋

Am I right in thinking that S3FileAdmin always works with posix paths, and that AzureFileAdmin is the only one that needs to be able to understand it might be using Windows for the remote filesystem? If you pass on_windows=True to S3FileAdmin is that always going to be broken?

If so then I think on_windows should be part of AzureFileAdmin only. Adding on_windows to the BaseFileAdmin doesn't feel right to me, unless I'm missing something.

[samialfattani]

Am I right in thinking that S3FileAdmin always works with posix paths, and that AzureFileAdmin is the only one that needs to be able to understand it might be using Windows for the remote filesystem?

Yes, u r right

If you pass on_windows=True to S3FileAdmin is that always going to be broken?

Yes it will

Adding on_windows to the BaseFileAdmin doesn't feel right to me, unless I'm missing something.

OK then can you tell me how the following example can work on Windows host without on_windows being passed to BaseFileAdmin?

import os
from io import BytesIO

import boto3
from flask import Flask
from flask_admin import Admin
from flask_admin.contrib.fileadmin import FileAdmin
from flask_admin.contrib.fileadmin.s3 import S3FileAdmin
from flask_babel import Babel
from testcontainers.localstack import LocalStackContainer

app = Flask(__name__)
app.config["SECRET_KEY"] = "secret"
admin = Admin(app, name="Example: S3 File Admin")
babel = Babel(app)


if __name__ == "__main__":
    with LocalStackContainer(image="localstack/localstack:latest") as localstack:
        s3_endpoint = localstack.get_url()
        os.environ["AWS_ENDPOINT_OVERRIDE"] = s3_endpoint

        s3_client = boto3.client(
            "s3", aws_access_key_id="test", aws_secret_access_key="test", endpoint_url=s3_endpoint,
        )

        bucket_name = "bucket"
        s3_client.create_bucket(Bucket=bucket_name)


        # BaseFileAdmin Must use posix (Alwasy Linux)
        admin.add_view( S3FileAdmin( bucket_name=bucket_name,  s3_client=s3_client )

       # BaseFileAdmin Must use os.path.normpath() (currently Windows)
        admin.add_view( FileAdmin('localdir', name='Local Dir') )

        app.run(debug=True)

if we run this code on Windows host, then BaseFileAdmin will work executed twice (once with on_windws=False to browse S3, and the other one with on_windows=True to browse the local directory that is placed on Windows).

maybe you are just get confused with the naming!! if we renamed the parameter into storage_is_on_windws, dese make since now? BaseFileAdmin need to know what the storage platform is regardless if the host platform it self.

[samuelhwilliams]

Looking a bit closer, I think it's a mistake for BaseFileAdmin to know anything about the OS it's on; that's what the *Storage classes seem to be for (LocalFileStorage, AzureStorage, S3Storage).

I think:

• LocalFileStorage should always use os.sep and/or os.path.join` operations, which will automatically detect the local OS and use the correct thing
• AzureStorage is the one that needs to know if it's on Windows or linux/posix/unix/whatever
• S3Storage should treat as always on posix

[samialfattani]

Totally agree with you , and now to let this happend we need to move _normpath() function from BaseFileAdmin into storage classes.

What i suggest is to create a BaseStorage class to a parent for all storage classes. It should includes normpath() function and on_windows . This way we can configure each storage to its appropriate platform and BaseFileAdmin has nothing to do with normalizing paths.

if you confirm the idea , i will start the implmentation right away and add push a new commit

[samialfattani]

i hope now it is ready for test and approve