Feature #1767 - RegexSwap client reimplementation based on JSON endpoint #1896
Conversation
| final ObjectMapper objectMapper = new ObjectMapper(); | ||
| root = objectMapper.readValue(json, Map.class); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); |
There was a problem hiding this comment.
Shouldn't this warn the user, or is it handled somewhere else?
There was a problem hiding this comment.
I didn't really mean "more specific". I meant more like a proper "we couldn't contact the server" local warning instead of just dumping it up the chain.
But I might misremember how we handle exceptions and this is all handled properly somewhere (though "someone will probably handle it" seems like a bad strategy).
There was a problem hiding this comment.
I see what you mean, but since this is a engine/backend component I think that throwing an exception is most appropriate. We could add more logging and/or make a specific exception for this scenario. But then again I do kinda think this is also just about wrapping the checked exception in an unchecked exception and deciding not to try and handle it, because we have no good way to recover from it here.
| } | ||
| return regexes; | ||
| } | ||
| private static final URI REGEXES_URI = URI.create("https://datacleaner.github.io/content/regexes.json"); |
There was a problem hiding this comment.
Leaving the URL as a hardcoded value may leave us open to a similar issue again later.
There was a problem hiding this comment.
Hmm yes but I find it hard to imagine a situation where we wouldn't have to pull a release to fix it anyway?
Is your suggestion that we use an environment variable or something like that?
There was a problem hiding this comment.
Yeah, just some kind of fallback. I guess an environment variable that overrides the default would be perfect for that.
Just so that someone who doesn't want to update for compatibility or whatever reason (or who wants a custom set of regexes hosted locally), can update the URL.
But its no biggie.
There was a problem hiding this comment.
Good point, fixed.
|
Code itself looks mostly good to me, but I see some changes from spaces to tabs in |
OMG yes. This is what happens when you (I) don't code for a long time and go rogue with a fresh Eclipse installation :D Thanks for the feedback notes! |
There was a problem hiding this comment.
No real blockers, though I'm not sure I agree with the exception strategy. But if I remember correctly, this is just using the new-ish Oracle approved way to strip checked exceptions that we (unfortunately) already did in several places, so I guess it's not really a step backwards.
As described and discussed in #1767 - this changes the RegexSwapClient class to use https://datacleaner.github.io/content/regexes.json for fetching regexes instead of the decomissioned old XML endpoints.