-
Notifications
You must be signed in to change notification settings - Fork 641
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
reject empty readme file & fix error message for invalid extension #8464
Conversation
Should we also consider fixing the JS rather than just reject this kind of packages? |
Main reason to reject empty pakcage is becasue we want to be consistent with what client does validation during pack. |
src/NuGetGallery/Strings.resx
Outdated
@@ -1166,7 +1166,7 @@ The {1} Team</value> | |||
<value>The <readme> element is not currently supported.</value> | |||
</data> | |||
<data name="UploadPackage_InvalidReadmeFileExtension" xml:space="preserve"> | |||
<value>The readme file has an invalid extension '{0}'. Extension must be one of the following: {1}.</value> | |||
<value>The readme file has an invalid extension '{0}'. Extension must be end in: {1}.</value> |
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.
What do you think of this?
<value>The readme file has an invalid extension '{0}'. Extension must be end in: {1}.</value> | |
<value>The readme file has an invalid extension '{0}'. The extension must be: '{1}'.</value> |
src/NuGetGallery/Strings.resx
Outdated
@@ -1215,4 +1215,7 @@ The {1} Team</value> | |||
<data name="UploadPackage_OwnerlessIdNamespaceConflictHtml" xml:space="preserve"> | |||
<value>The package ID is reserved. You can upload your package with a different package ID. Reach out to <a href="mailto:support@nuget.org">support@nuget.org</a> if you have questions.</value> | |||
</data> | |||
<data name="ReadmeErrorEmpty" xml:space="preserve"> | |||
<value>The readme file '{0}' cannot be empty</value> |
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.
Should this end with a period?
<value>The readme file '{0}' cannot be empty</value> | |
<value>The readme file '{0}' cannot be empty.</value> | |
@@ -458,6 +458,16 @@ private async Task<PackageValidationResult> CheckReadmeMetadataAsync(PackageArch | |||
} | |||
|
|||
var readmeFileEntry = nuGetPackage.GetEntry(readmeFilePath); | |||
|
|||
if (readmeFileEntry.Length == 0) |
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.
What about a readme with just whitespace? Would it be worth rejecting those too?
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.
good question, probably not rejecting those? @chgill-MSFT what do you think?
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.
We should keep a consistent behavior on validations between the server and client.
If the client accepts a whitespace readme but the server rejects it, it's not a good customer experience.
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.
Our validations are already a superset of client, by design. I don't think an empty string README is useful. But also, it's the author's fault if they do this. My guess it is likely unintentional and not a helpful content to the user. But I don't feel strongly.
Summary of the changes (in less than 80 characters):
reject empty readme file:
correct error message for invalid extension:
Addresses https://github.com/NuGet/Engineering/issues/3707, #8460