Skip to content
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

Handle missing vehicle_types_available gracefully #257

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

hbruch
Copy link
Collaborator

@hbruch hbruch commented Sep 13, 2023

This PR partly addresses #250 (partly) by treating missing station.vehicle_types_available as empty arrays.
Note: it does not provide better loggging, but only fixes an ingestion exception with missing vehicle_types_available.

In consequence, no exceptions are logged for e.g. this feed configuration:

lamassu:
  providers:
    - systemId: lime_zu
      operatorId: LME:Operator:lime
      operatorName: Lime
      codespace: DKY
      url: "https://data.lime.bike/api/partners/v2/gbfs/zug/gbfs.json"
      language: en

and the following queries returned expected results:

GBFS queries tested

http://localhost:8080/gbfs/lime_zu/gbfs
http://localhost:8080/gbfs/lime_zu/station_status
http://localhost:8080/gbfs/lime_zu/station_information

Before: Whitelabel Error page
After: gfbs/station_status/station_information is shown

http://localhost:8080/validation/systems/lime_zu
Before: []
After: Validation reporting #/data/stations/0: required key [vehicle_types_available] not found

GrapqhiQL Queries tested

{operators {
  id
}}

returned operator

{stations(range: 10000000, lon: 8.516, lat: 47.1528) {
id
}}

Did not returns any station though, as pricing_plans are provided by this feed.

@testower is there a way to configure pricing_plans in lamassu, so the stations can be cached anyway? I tested without success:

lamassu:
  providers:
    - systemId: lime_zu
      operatorId: LME:Operator:lime
      operatorName: Lime
      codespace: DKY
      url: "https://data.lime.bike/api/partners/v2/gbfs/zug/gbfs.json"
      language: en
      pricing_plans:
        planId: TestPlan
        name: TestPlan
        price: 0.0
        isTaxable: False
        currency: EUR

@testower
Copy link
Collaborator

testower commented Sep 13, 2023

@testower is there a way to configure pricing_plans in lamassu, so the stations can be cached anyway? I tested without success:

Yes you can provide pricingPlans, it follows the v2.3 model (GBFSPlan) see https://github.com/entur/lamassu/blob/master/src/main/java/org/entur/lamassu/model/provider/FeedProvider.java#L39

I think you had it almost right, but mistype pricingPlans and it needs to be an array:

lamassu:
  providers:
    - systemId: lime_zu
      operatorId: LME:Operator:lime
      operatorName: Lime
      codespace: DKY
      url: "https://data.lime.bike/api/partners/v2/gbfs/zug/gbfs.json"
      language: en
      pricingPlans:
        - planId: TestPlan
          name: TestPlan
          price: 0.0
          isTaxable: False
          currency: EUR

@hbruch
Copy link
Collaborator Author

hbruch commented Sep 13, 2023

Thanks! Yes, with this config,

{
  stations(range: 100, lon: 8.516, lat: 47.1528) {
    id
    name {
      translation {
        language
        value
      }
    }
    lat
    lon
    vehicleTypesAvailable {
      vehicleType {
        id
        formFactor
      }
      count
    }
  }
}

successfully returns the station also:

{
  "data": {
    "stations": [
      {
        "id": "DKY:Station:zug",
        "name": {
          "translation": [
            {
              "language": "en",
              "value": "Zug"
            }
          ]
        },
        "lat": 47.1528,
        "lon": 8.516,
        "vehicleTypesAvailable": null
      }
    ]
  }
}

@testower
Copy link
Collaborator

I'm making a mental note that there's no code quality check or test coverage reporting for PRs from forks :/

It would be nice to see some test coverage on how this change affects the filtering - perhaps in SpatialIndexIdFilterTest - would you be up for it?

@hbruch
Copy link
Collaborator Author

hbruch commented Sep 13, 2023

I added a test which fails with a NPE (in SpatialIndexIdUtil, not SpatialIndexIdFilter) without the changes of this PR, and is green with this fix.

Note: I tried to match the naming conventions of preexisting tests, though I personally wouldn't name stub builder methods testStubObject() but stubObject() or buildStubObject() (at first I misread them as test methods). But this is a question unrelated to this PR.

@testower
Copy link
Collaborator

Note: I tried to match the naming conventions of preexisting tests, though I personally wouldn't name stub builder methods testStubObject() but stubObject() or buildStubObject() (at first I misread them as test methods). But this is a question unrelated to this PR.

Good observation, I agree these should be renamed. If you want you could do it in this PR or separately.

@hbruch
Copy link
Collaborator Author

hbruch commented Sep 20, 2023

I renamed the builder methods' prefix from test to a.
If you are ok with this change, this PR would be ready for review/merge from my side.

@testower
Copy link
Collaborator

If you don't mind, just a rebase on master and we're good to go.

@testower testower merged commit ad76781 into entur:master Sep 20, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants