-
Notifications
You must be signed in to change notification settings - Fork 21
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
Fail base64 validation if unrecognized characters are found #5
base: main
Are you sure you want to change the base?
Conversation
xsdlib/src/main/java/com/sun/msv/datatype/xsd/Base64BinaryType.java
Outdated
Show resolved
Hide resolved
@iglosiggio A thousand thanks for this patch! Allow me to fix the enabling of the tests first, I was not aware! PS: I beg your pardon, that I have overseen the patches in my email (they are not yet in my "primary" folder of GMail, but in "updates" with commercials) I added an item to my ToDo list to mark GitHub notifications as important! |
For instance, for XSDLIB Git would show the following file movements: |
After moving the XML files from src/test/java to src/test/resources they will appear in target test class directory Earlier you will find only class files: |
@iglosiggio Could you please try to update your branch/pull request? PS: When you test, the error message "CatalogManager.properties: catalogs not found." happened earlier as well, therefore I still think the *.properties belong to the src/main/resources directory. |
As the update of a pull request not by the owner is not intuitively documented (uncertain this is possible at all at GitHub), I have created a new rebased branch, where your issues for #4 are within: What concerns me is that there are some errors thrown in the newly activated test, although it seems not related to your change, but regarding data types. Can you or anyone dive there a bit deeper? I would like to do a new MSV release soon, but the new errors do not look so good (even if the test had been disabled earlier) ;-) Thank you in advance, PS: I renamed the test class back "removing again its new suffix 'Test'", as I remembered that explicit names could be added to the pom.xml of Maven to be executed by the Surefire test plugin! |
@iglosiggio I just read beyond the box, that I simply would have cloned your repo and made a pull request there, to update this pull request! Now I know for the next time! :-) |
Thanks for all the comments @svanteschubert! I'll look into this tomorrow 😅 |
ba7b248
to
49e0dce
Compare
@svanteschubert just checked on our sw and it worked perfectly! I've just reseted my branch personal so it points at your rebase (feel free to merge either one). |
@iglosiggio Thanks a lot, Ignacio! Which I have not attached to failing a JUnit test (the build would break). Thanks in advance, |
I have added some JUnit Assert.fail(..) to the tests to make our problem a little more obvious: Earlier these tests were not executed, I was not aware of them until you fixed one. |
Several other implementations convert IllegalArgumentException to returning null, so do the same here. This fixes some DataTypeTest.
It's not very obvious how this should work, but it is obvious that this can't ever have passed completely. There is one definite bug there where negative gYear is decremented by 1 on roundtrip. Eventually ran out of time so disabled about a third of the tests altogether.
i did try to fix the test, it's quite confusing, i found 2 bugs, fixed one of them, disabled lots of test inputs, ran out of time... |
This should fix #4. I've changed the conformance tests accordingly but these aren't enabled by default during compilation (and some are failing).