Catch client exception and rethrow to avoid loader failure to crash otp

[testower]

Summary

This fixes a bug likely introduced by #6735 , that causes OTP to crash if the gbfs loader fails to load a feed, e.g. due to dns failure or similar. The problem was the the http client exception was not caught anywhere. This PR catches it and rethrows it as a UpdaterConstructionException, which in turn ends up just logging a warning and skipping that updater.

Issue

Fixes #6912

Unit tests

Added a unit test which covers this fix in GbfsFeedMapperTest which seems to be a bit of a bucket test class for testing gbfs updater classes. We may want to clean that up a bit?

Documentation

No documentation as this is considered a bug fix.

Changelog

Skipping

Bumping the serialization version id

Not needed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.10%. Comparing base (cc6c6c6) to head (af96db6).
⚠️ Report is 140 commits behind head on dev-2.x.

Additional details and impacted files

@@            Coverage Diff             @@
##             dev-2.x    #6921   +/-   ##
==========================================
  Coverage      72.10%   72.10%           
+ Complexity     19660    19659    -1     
==========================================
  Files           2124     2124           
  Lines          79507    79510    +3     
  Branches        8050     8050           
==========================================
+ Hits           57326    57329    +3     
+ Misses         19344    19343    -1     
- Partials        2837     2838    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[optionsome]

Thanks, the fix seems to work. We just need to think a bit about how we want to test this (if we want to test it).

[optionsome]

Is this the correct place for this? There is also GbfsFeedLoaderTest which seems to have more tests similar to this. It has some real integration tests which are disabled (because we try to avoid those), but we usually avoid mocking things as well. I'm not sure what would be the best approach here. @leonardehrenfried has created a submodule for real integration tests which are run in CI if the pr has certain label. I wonder if that could be expanded with realtime updater related tests. Although, in this case a real integration test might not make any sense. I can bring this up in the dev meeting tomorrow.

There are still the tests from #4826, I'm not sure what we should do about those and if they really test anything.

[testower]

I put the test there just to avoid wasting any time figuring out a better place for it - knowing that we might not even want the test in the first place. Personally, I don't think it's needed, but at least it proved that my change did what I expected it to do.

[leonardehrenfried]

We have quite a bit of exception handling logic in the updaters:

OpenTripPlanner/application/src/main/java/org/opentripplanner/updater/vehicle_rental/datasources/gbfs/v2/GbfsFeedLoader.java

Lines 45 to 47 in e54d80b

	} catch (URISyntaxException e) {
	throw new UpdaterConstructionException("Invalid url " + url);
	}

I think what is different now is that we used to not make HTTP calls during setup but now we do, because we must detect the version. That is also the reason why the test didn't do anything - it predates the setup method.

I think we ought to spend a little bit of time thinking about how we want to do the error handling and testing.

[leonardehrenfried]

Since @testower approved it, I'm closing this in favour of #6954