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

implement get file stream for writing #33

Merged
merged 9 commits into from
Oct 1, 2024
Merged

Conversation

garcipat
Copy link

@garcipat garcipat commented Sep 11, 2023

I implemented the new method for getting a stream to write files.

Can you please give feedback regarding the type of exception and also on how to handle the ReadWrite value that works on FileSystem but not for azure since it does only allow either read or write.

Also I dont even know if I can test this locally or only with the actions

@CLAassistant
Copy link

CLAassistant commented Sep 11, 2023

CLA assistant check
All committers have signed the CLA.

@ejsmith
Copy link
Contributor

ejsmith commented Sep 11, 2023

Thanks @garcipat! For the ReadWrite thing, I think basically we want to be able to read a file, write a file (overwrite if it exists) and append to a file which is what ReadWrite is currently doing, correct? I'm thinking maybe we should be more explicit about that and change up the enum we are using for getting a stream in the higher foundatio library to reflect those actions. Then for Azure, I think they have an append feature and would imagine most storage implementations would be able to append as well.

For testing, it is using a storage emulator in the docker-compose.yml file at the root. So if you docker compose up in the root, you will have a running storage emulator.

@garcipat
Copy link
Author

Okay. Should I do another PR in the main repo first then implementing it for the other providers?
Will check the append thing. For files both read and write are supported. Should that not be exposed because its too specifit to the file system?

@ejsmith
Copy link
Contributor

ejsmith commented Sep 11, 2023

Maybe try implementing ReadWrite as append in azure and make sure that works before we go and change the core Foundatio stream abstraction?

@garcipat
Copy link
Author

Sure. As soon as I get back tomorrow.

@garcipat
Copy link
Author

garcipat commented Sep 14, 2023

Okay I tried too get this to work with a read/write stream but i think Azure (maybe as well others) dont allow read and write at the same time.

To explain a little bit what I tried to do any why I'm workin gon that is, that Im having an ZIP archive in file storage. But ZipArchive requires a stream that can read, write and seek. I even tried to write my own stream but It seems that this is not feasable. But wiht local file storage this works fine. I now just dont know how it would be best to have the interface. Only exposing Read and Write is losing the advantage of having both if you are working with FileSystem. But exposing ReadWrite maybe is only supported by a few.

Its not a big issue since I'm extending the IFileStorage interface anyway and also the implementations in my code so I can change the behaviour if I like to. But since I was working on it anyway i thought why not to contribute and making that available for others.

Tell me what you think would be best. I could also check a few other providers like AWS and see if those can handle it and its maybe only an issue on azure.

@ejsmith
Copy link
Contributor

ejsmith commented Sep 15, 2023

@garcipat Hmm... So I guess in the zip scenario you wouldn't want to just append to a file, you'd actually want to seek and overwrite portions of the file, correct?

I've seen a few articles online that seemed to be saying you could seek to different blocks and then overwrite that entire block. One problem is that I think our Azure storage implementation is using an outdated client that we need to get upgraded. Wonder if the newer library would support it?

I guess we need to figure out if a majority of blob storage implementations would support that scenario and if so then it would make sense to keep the abstraction as is with a ReadWrite option and we would just have to throw a not implemented exception in ones that didn't support seeking. I guess we need to do some research and find out what the support level is.

I see this article for S3 indicating that it does not support seekable streams by default, but maybe there are workarounds:
https://medium.com/circuitpeople/random-access-seekable-streams-for-amazon-s3-in-c-bd2414255dcd

I saw a similar one for Azure, but can't seem to find it.

Have you found any info?

It seems like even an append operation isn't implemented in S3 and probably others since they are S3 compatible. Which makes me feel like we should keep this abstraction simple for now and change to use our own enum that just has read and write stream support and we can always add additional modes in the future since we will be using our own enum.

@garcipat
Copy link
Author

garcipat commented Sep 15, 2023 via email

@garcipat
Copy link
Author

Are the license agreement checks broken? I signed it but it just wont detect it.

@ejsmith
Copy link
Contributor

ejsmith commented Sep 24, 2023

I just logged in and checked configuration and they seem to be configured right, but the site was super slow. I wonder if they are having issues.

@garcipat
Copy link
Author

I just logged in and checked configuration and they seem to be configured right, but the site was super slow. I wonder if they are having issues.

I only had issues on the AzureFoundation one. The other in the base repo worked fine.

@garcipat
Copy link
Author

I just logged in and checked configuration and they seem to be configured right, but the site was super slow. I wonder if they are having issues.

I only had issues on the AzureFoundation one. The other in the base repo worked fine.

I found out what the issue was. I pushed things with my github account of my project I'm working in. Therefore I had to also sign it with that account. Need to pay attention not to push with that one.

…Storage

# Conflicts:
#	src/Foundatio.AzureStorage/Foundatio.AzureStorage.csproj
#	src/Foundatio.AzureStorage/Storage/AzureFileStorage.cs
#	tests/Directory.Build.props
@garcipat
Copy link
Author

garcipat commented Oct 1, 2024

Hey @ejsmith, I found time to update this PR. For now just the writing was added and should work just fine, also the test for it.

The ReadWrite can still be tackled separatly I thought.

@niemyjski niemyjski self-assigned this Oct 1, 2024
@niemyjski niemyjski merged commit 6e62311 into FoundatioFx:main Oct 1, 2024
2 checks passed
@niemyjski
Copy link
Member

Thanks so much for the PR!

@garcipat
Copy link
Author

garcipat commented Oct 2, 2024

I happy to contribute. I well see if I can also do that with the AWS package if not already done.

@niemyjski
Copy link
Member

That would be awesome! It doesn't yet support it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants