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

Try seek blob stream position to begin #10445

Merged
merged 4 commits into from Oct 27, 2021
Merged

Try seek blob stream position to begin #10445

merged 4 commits into from Oct 27, 2021

Conversation

JadynWong
Copy link
Contributor

Use the file system or azure blob the blob provider, the Position of the returned stream is at the end.

@EngincanV EngincanV added this to the 5.0-preview milestone Oct 27, 2021
@EngincanV EngincanV requested a review from cotur October 27, 2021 06:16
@maliming
Copy link
Member

We should change blob provider instead of CmsKit.

Copy link
Contributor

@cotur cotur left a comment

Choose a reason for hiding this comment

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

Hi @JadynWong,

as @maliming said, we need to upgrade the Blob Providers instead of updating it at the Cms-Kit module.

@JadynWong JadynWong changed the title Fix CmsKit media download Try seek blob stream position to begin Oct 27, 2021
@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #10445 (8fbb765) into dev (08f2482) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 8fbb765 differs from pull request most recent head c8e6316. Consider uploading reports for the commit c8e6316 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##              dev   #10445   +/-   ##
=======================================
  Coverage   54.73%   54.73%           
=======================================
  Files        2641     2641           
  Lines       75354    75361    +7     
=======================================
+ Hits        41243    41250    +7     
  Misses      34111    34111           
Impacted Files Coverage Δ
....BlobStoring/Volo/Abp/BlobStoring/BlobContainer.cs 97.84% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08f2482...c8e6316. Read the comment docs.

@JadynWong
Copy link
Contributor Author

Hi @maliming

I have some questions.
Now I try to handle it in BlobContainer's GetOrNullAsync. Users usually use the IBlobContainer to manipulate the blob, and the BlobProvider only as an implementation.
Do we need to move the TrySeekStreamToBegin method to the BlobProviderBase, and handle it in each Provider?

@maliming
Copy link
Member

We are using MemoryStream, we can change it directly.

No need for the TrySeekStreamToBegin method

@JadynWong
Copy link
Contributor Author

DatabaseBlobProvider using byte[] does not require it, it is default at start position

@maliming maliming merged commit 5e171e6 into abpframework:dev Oct 27, 2021
@JadynWong JadynWong deleted the jadyn/fix-cmskit-media-download branch October 27, 2021 16:01
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.

None yet

5 participants