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.

Sign up for GitHub

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

Jump to bottom

[Storage][Webjobs] Hide blob name validation attribute. #15794

Merged
merged 1 commit into from
Oct 8, 2020
Merged

[Storage][Webjobs] Hide blob name validation attribute. #15794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions ...age.Webjobs.Extensions.Blobs/api/Azure.WebJobs.Extensions.Storage.Blobs.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ public BlobAttribute(string blobPath) { }
public BlobAttribute(string blobPath, System.IO.FileAccess access) { }
public System.IO.FileAccess? Access { get { throw null; } set { } }
[Microsoft.Azure.WebJobs.Description.AutoResolveAttribute]
[Microsoft.Azure.WebJobs.Description.BlobNameValidationAttribute]
public string BlobPath { get { throw null; } }
public string Connection { get { throw null; } set { } }
}
Expand All @@ -25,16 +24,6 @@ public BlobTriggerAttribute(string blobPath) { }
public string Connection { get { throw null; } set { } }
}
}
namespace Microsoft.Azure.WebJobs.Description
{
public partial class BlobNameValidationAttribute : System.ComponentModel.DataAnnotations.ValidationAttribute
{
public BlobNameValidationAttribute() { }
protected override System.ComponentModel.DataAnnotations.ValidationResult IsValid(object value, System.ComponentModel.DataAnnotations.ValidationContext validationContext) { throw null; }
public static bool IsValidBlobName(string blobName, out string errorMessage) { throw null; }
public static bool IsValidContainerName(string containerName) { throw null; }
}
}
namespace Microsoft.Azure.WebJobs.Extensions.Storage
{
public partial class AzureStorageBlobsWebJobsStartup : Microsoft.Azure.WebJobs.Hosting.IWebJobsStartup
Expand Down
6 changes: 3 additions & 3 deletions sdk/storage/Azure.Storage.Webjobs.Extensions.Blobs/src/BlobNameValidationAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace Microsoft.Azure.WebJobs.Description
/// Validate this on the client side so that we can get a user-friendly error rather than a 400 from the service.
/// See code here: http://social.msdn.microsoft.com/Forums/en-GB/windowsazuredata/thread/d364761b-6d9d-4c15-8353-46c6719a3392
/// </summary>
public class BlobNameValidationAttribute : ValidationAttribute
internal sealed class BlobNameValidationAttribute : ValidationAttribute
Copy link
Member

Choose a reason for hiding this comment

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

Right - because where these are validated https://github.com/Azure/azure-webjobs-sdk/blob/ccc20de1fb7fd7043aed6d12a2c46acce1d7a49a/src/Microsoft.Azure.WebJobs.Host/Bindings/AttributeCloner.cs#L199 we look for ValidationAttribute.

Copy link
Member

Choose a reason for hiding this comment

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

Did you not carry the PublicSurface tests forward to this new repo? https://github.com/Azure/azure-webjobs-sdk/blob/5e410d7ae1c7c1b54b81b6a50faa5cc226deba5e/test/Microsoft.Azure.Webjobs.Extensions.Storage.UnitTests/PublicSurfaceTests.cs#L55. We had a set of useful tests that helped us verify public surface area.

Copy link
Contributor Author

@kasobol-msft kasobol-msft Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I did initially (See #15381 (comment) ). However the azure-sdk-for-net has an infrastructure to extract public surface into files like /api/Azure.WebJobs.Extensions.Storage.Blobs.netstandard2.0.cs (included in this diff) and warn against public area changes.
There's also a process to review public surface area before shipping any public preview or GA which is based on these extracts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's more about the process https://github.com/Azure/azure-sdk/blob/master/docs/policies/reviewprocess.md

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks

{
// Tested against storage service on July 2016. All other unsafe and reserved characters work fine.
private static readonly char[] UnsafeBlobNameCharacters = { '\\' };
Expand Down Expand Up @@ -83,7 +83,7 @@ protected override ValidationResult IsValid(object value, ValidationContext vali
/// </summary>
/// <param name="containerName"></param>
/// <returns></returns>
public static bool IsValidContainerName(string containerName)
internal static bool IsValidContainerName(string containerName)
{
if (containerName == null)
{
Expand All @@ -105,7 +105,7 @@ public static bool IsValidContainerName(string containerName)
/// <param name="blobName"></param>
/// <param name="errorMessage"></param>
/// <returns></returns>
public static bool IsValidBlobName(string blobName, out string errorMessage)
internal static bool IsValidBlobName(string blobName, out string errorMessage)
{
const string UnsafeCharactersMessage =
"The given blob name '{0}' contain illegal characters. A blob name cannot the following character(s): '\\'.";
Expand Down