HTTPS URL Property Validator #2079
Conversation
| return null; | ||
| } | ||
|
|
||
| if (!"https".equalsIgnoreCase(url.getProtocol())) { |
There was a problem hiding this comment.
I think Yannick build recently some central checks about http checks, would be good to reuse those.
There was a problem hiding this comment.
Only if there is a non-trivial benefit. URL parsing is securely done via a Standard Library method, leaving only the protocol comparison as "custom code". IMHO, that is too trivial to invoke yet another helper.
There was a problem hiding this comment.
Hm, I don't see it this way. It was done exactly to avoid 1.000 places checking if a string starts with http. BTW, its in Strings.isHttpUrl. but it does not enforce https and allows http as well 🫣
My vote is to add a isHttpsUrl method among with it, but its just my opinion.
There was a problem hiding this comment.
…and it does not even check whether it is a valid URL after the prefix at all, accepting https:// ;%@@ lol whatever i don't care as valid URL, at the same time (ab-)using fully-fledged pattern matching for a simple case-insensitive prefix match. And that is the kind of faulty code we should use everywhere instead of standard methods?
We are approaching the test week. We need to get stuff done. Basically, you attempt to force me into bugfixing another project, open another PR there, wait for it to be processed, update this here, so that I can finally continue the work I am supposed to have done in time for the patch, just to move perfectly fine and effectively trivial code from this PR somewhere else. Honestly, under these circumstances, we really can save the discussions in future retros about planning and extra-hours and do something more productive instead.
But fine, then I'll work on sirius-kernel instead of the OX ticket.
…as requested by @idlira during review and after fixing the respective implementation. OX-11244
|
|
||
| @Override | ||
| public void beforeSave(Property property, Object value) { | ||
| if (value instanceof String url && Strings.isFilled(url) && !Strings.isHttpsUrl(url)) { |
There was a problem hiding this comment.
Very optional: Could reduce duplication by extracting the actual check/validation (the if-condition) into a private method
There was a problem hiding this comment.
The variable url created in the first condition is used within the block for creating the error message. Hence, in order to extract a method, it would be something like a attemptToParseInvalidString(value) returning an Optional. Not sure whether that would be easier to understand and maintain.
However, like the other validators, this validator currently only checks Strings, and entirely ignores other data types. Maybe we could also get rid of the instance-of and generally use .toString(). That would make extraction of the method trivial.
Thoughts?
There was a problem hiding this comment.
Ah right. I think its okay for now then
Description
Adds a property validator that enforces a valid HTTPS URL.
Note that the entire property validation framework may be subject to some maintenance soon in order to improve integration with Tycho (and file structure). See SIRI-1050.
Additional Notes
Checklist