-
Notifications
You must be signed in to change notification settings - Fork 846
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
Fix the error of the opening the content editing blade #1762
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- some potential problems in code with an unhandled exception.
- responsibility violations. if we open streams and return them to caller code we should implement https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1063?view=vs-2019
what will happen if we forgot place OpenRead into using a statement? I think the stream stays alive until the next garbage collection, even survive after. It's might be the reason for the memory leaks!
also, see this https://stackoverflow.com/questions/32058317/how-to-return-a-stream-from-a-method-knowing-it-should-be-disposed
-
should be removed
public const string DefaultBlobContainerName = "default-container"; -
misspellinghttps://github.com/VirtoCommerce/vc-platform/blob/8b4e4cb5a9140f9185d662a36a9f8698aa3a0a18/VirtoCommerce.Platform.Data.Azure/AzureBlobProvider.cs#L279
-
should be renamed relativeUrl to blobKey in accordance with an interface declaration
public string GetAbsoluteUrl(string relativeUrl) -
fix braces
await sourse.DeleteIfExistsAsync(); -
hardcoded
blob.Properties.CacheControl = "public, max-age=604800"; -
URIs shouldn't be hardcoded
var path = (folder.ParentUrl != null ? folder.ParentUrl + "/" : string.Empty) + folder.Name; -
obvious comments
// Retrieve container reference.
@@ -375,6 +352,24 @@ private CloudBlobContainer GetBlobContainer(string name) | |||
return retVal; | |||
} | |||
|
|||
private BlobInfo ConvertBlobToBlobInfo(ICloudBlob cloudBlob) | |||
{ | |||
var relativeUrl = cloudBlob.Uri.LocalPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that RelativeUrl
has a different setter logic in the old places and it is skipped during refactoring.
https://github.com/VirtoCommerce/vc-platform/pull/1762/files#diff-d58e38c347dafea607a39a0204759789L186
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, GetBlobInfo and Search methods had different logic for obtaining RelativeUrl. The difference is that in the GetBlobInfo method, the resulting RelativeUrl value has a leading slash, and in the Search method, the RelativeUrl value did not have a leading slash. Now the logic is the same. With a leading slash.
SonarQube analysis reported 1 issue Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
https://github.com/VirtoCommerce/vc-platform/issues/1760#?repos=60280313&search=1060
To avoid case with the wrong content type of the blob, when it
uses AzureBlobProvider for CMS content, better get content type
on a fly at BlobInfo returning. The wrong type can be formed
when CMS data imported direct to Azure Blob Storage.