entur / lamassu

Mobility hub
European Union Public License 1.2
5 stars 7 forks source link

Some feeds are reported as app_lamassu_gbfs_validation_missingrequiredfile though feed exists #430

Closed hbruch closed 2 months ago

hbruch commented 3 months ago

Expected behavior

For valid feeds, like e.g. this donkey feed, lamassu should not report app_lamassu_gbfs_validation_missingrequiredfile.

Observed behavior

With current Lamassu commit 0ddee08, starting from 02a755a (merge of consume-v3), we receive

app_lamassu_gbfs_validation_missingrequiredfile{file="gbfs",system="donkey_ge",version="2.3",} 1.0

Version of lamassu used (exact commit hash or JAR name)

0ddee08

Data sets in use (links to GBFS feeds)

https://stables.donkey.bike/api/public/gbfs/2/donkey_ge/gbfs.json

Configuration used

lamassu:
  providers:
    - systemId: donkey_ge
      operatorId: DKY:Operator:donkey
      operatorName: Donkey Republic
      codespace: DKY
      url: "https://stables.donkey.bike/api/public/gbfs/2/donkey_ge/gbfs.json"
      language: de
hbruch commented 3 months ago

Reason for this issue seems that with 1ab4111 GBFSLoader.getRawFeed had the following special case removed:

    if (feedName.equals(GBFSFeedName.GBFS)) {
      return rawDiscoveryFileData;
    }

I'm unsure, why this was necessary and would suggest to re-add e.g. to BaseGbfsLoader

    public Optional<byte[]> getRawFeed(S feedName) {
      if (feedName == getDiscoveryFeedName()) {
        return discoveryFileUpdater.getRawData();
      }

      GBFSFeedUpdater<?> updater = feedUpdaters.get(feedName);
      if (updater == null) {
        return Optional.empty();
      }
      return updater.getRawData();
    }

    protected abstract S getDiscoveryFeedName();
testower commented 3 months ago

Good catch! I think this happened because I originally had the intention of adding the discovery file updater to the list of updaters, so it would be refreshed along with the others. In the end, I decided to save that for later, but forgot to fix this case. Your suggestion looks correct!

hbruch commented 2 months ago

Ah, I see. Then an alternative fix could be to move forward and have the discovery feed added to getFeeds() and get it updated regularly, no?

testower commented 2 months ago

One of the reasons I decided to save this for later is that it would require some design thinking about how potential changes in the discovery file should affect the other updaters. I think your fix is good for now.

testower commented 2 months ago

Let's create a separate issue for updating the discovery file itself. I suppose it has to add updaters for new files, and remove updaters for files that are removed. Doesn't sound too complicated, but still I think it should be handled separately from this issue which is actually a bug in the current code.

I guess it also has to take into account possible changes in URLs of files ...

testower commented 2 months ago

Reopened - will close when lib has been upgraded in lamassu.