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

(doc) [IO-484] Fix incorrect FilenameUtils documentation for null bytes #310

Merged
merged 3 commits into from Jan 30, 2022

Conversation

Marcono1234
Copy link
Contributor

During the development of IO-484 the behavior was first to remove null bytes (afe78a0) but then later the implementation was changed to throw an exception instead (5d072ef).
However, not the complete documentation was adjusted.

This pull request corrects the documentation by mentioning that an IllegalArgumentException will be thrown for null bytes.

However, there are cases where the null bytes check is missing, or might be undesired. I have added TODO comments at these locations. Please let me know if you want any additional changes there.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 87.655% when pulling 511e77e on Marcono1234:IO-484-null-byte-exception into be83447 on apache:master.

@garydgregory
Copy link
Member

garydgregory commented Jan 11, 2022

Hi @Marcono1234
Thank you for your patience. The new docs are not quite right because we are not checking for null bytes, we are checking for \u0000 which is bigger than a byte, it's a char.

@garydgregory
Copy link
Member

@Marcono1234 ping

@Marcono1234
Copy link
Contributor Author

You are right, I have used the term "null character" now, which is also used by Wikipedia and in some of the documentation comments of the JDK classes.
Is that what you had in mind?

Also, what should I do regarding the TODO comments I added, should I just leave them in?

@garydgregory
Copy link
Member

garydgregory commented Jan 16, 2022

@Marcono1234
Thank you for the update. Yes, much better now. I think we can take out the TODO comments for now and make this a Javadoc only PR. We can revisit error checking later as this will require more tests to either validate the current behavior or change it. TY! 😀

@Marcono1234
Copy link
Contributor Author

Ok, I have removed the comments now. Here are the issues the TODO comments were about, in case you or someone else wants to have a look at them in the future:

  • doGetFullPath(String, boolean): In some cases does not call (indirectly) requireNonNullChars
  • equals(String, String, boolean, IOCase): Normalization can throw IllegalArgumentException when file names contain null characters. Currently not documented, but not clear what the desired behavior is. Should exception be caught and false be returned? If exception should not be caught, then exception has to be documented, also for callers of this method. And should fileName2 then always be validated as well? Currently if normalization is requested but fileName1 is null, fileName2 is not checked for null characters.
  • getExtension(String): Does not call requireNonNullChars

@garydgregory garydgregory merged commit b6a2218 into apache:master Jan 30, 2022
@Marcono1234 Marcono1234 deleted the IO-484-null-byte-exception branch January 30, 2022 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants