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

Fixing #376 #460

Merged
merged 7 commits into from
Jun 29, 2023
Merged

Fixing #376 #460

merged 7 commits into from
Jun 29, 2023

Conversation

BanulaKumarage
Copy link
Contributor

  • Earlier file names with non-ASCII characters were converted to ASCII characters in fs.url(filename) line 321 in app/core.py But the file is saved without converting in line 320. Therefore, the file is unable to find.
  • As a fix, the file is saved in app/media by converting its non-ASCII characters to ASCII characters. Then issue Validating an SPDX file with colon in name fails #376 is fixed.

src/app/core.py Outdated
@@ -316,7 +317,7 @@ def license_validate_helper(request):
fs = FileSystemStorage(location=settings.MEDIA_ROOT +"/"+ folder,
base_url=urljoin(settings.MEDIA_URL, folder+'/')
)
filename = fs.save(myfile.name, myfile)
filename = fs.save(unicodedata.normalize('NFKD', myfile.name).encode('ASCII', 'ignore').decode('ASCII'), myfile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@BanulaKumarage Can you please explain the changes. There seems to be a lot of encode/decode happening. Since this is the main flow, don't want to merge until I understand what exactly it does.

Also, can you create a util to do this since this would be required at other places as well.

Also, having some UT for this would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unicodedata.normalize method, converts any special character into their equivalent base characters or combinations of base characters. (ex: converting é to e ) Then the text is converted to ASCII characters. But if there is any character cannot convert it is replaced with a empty string( done by ignore)
Finally, the resulting ASCII-encoded string is decoded back into a regular Python string, which can then be used as the filename. ( converting b'mariadblatest.spdx' to mariadblatest.spdx )

@rtgdk
Copy link
Collaborator

rtgdk commented Apr 16, 2023

Update PR heading as well.

@@ -316,7 +316,7 @@ def license_validate_helper(request):
fs = FileSystemStorage(location=settings.MEDIA_ROOT +"/"+ folder,
base_url=urljoin(settings.MEDIA_URL, folder+'/')
)
filename = fs.save(myfile.name, myfile)
filename = fs.save(utils.removeSpecialCharacters(myfile.name), myfile)
Copy link

Choose a reason for hiding this comment

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

1% of developers fix this issue

E501: line too long (82 > 79 characters)


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@rtgdk
Copy link
Collaborator

rtgdk commented Jun 29, 2023

This doesn't seem to work for me on Mac - Upload
Screenshot 2023-06-29 at 5 53 14 PM

Online tools also failing with colon in filename with these changes
Screenshot 2023-06-29 at 6 33 57 PM

I think it's better to substitute all illegal filename characters instead, will push the changes.

@rtgdk
Copy link
Collaborator

rtgdk commented Jun 29, 2023

Thanks @BanulaKumarage for all the contributions! Need to merge the fix asap so creating a new PR on top of these changes.

@goneall goneall merged commit f028eac into spdx:main Jun 29, 2023
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.

3 participants