entur / lamassu

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

Inconsistent case handling for non lowercase system_ids #427

Closed hbruch closed 4 months ago

hbruch commented 5 months ago

Expected behavior

Either lamassu should enforce lowercase system_ids in feedproviders.yml, or it should not lowercase the system_id when publishing feed urls.

Observed behavior

We configured a provider using it's original mixed case system_id (e.g. provider_TOWN).

Lamassu publishes this feed in http://lamassu:8080/gbfs as http://lamassu:8080/gbfs/provider_town/gbfs, which results in a 404 error when requested. http://lamassu:8080/gbfs/provider_TOWN/gbfs works.

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

https://github.com/entur/lamassu/commit/8cff0c895aa590d7b113b41c79a2d57972c5a3ec

Data sets in use (links to GBFS feeds)

any

Steps to reproduce the problem

Configure e.g.

lamassu:
  providers:
    - systemId: provider_TOWN
      operatorId: op:Operator:p
      operatorName: Provider
      codespace: op
      url: "https://provider.com/gbfs.json"
      language: en

http://localhost:8080/gbfs will respond with:

{"systems":[{"id":"provider_TOWN","url":"http://localhost:8080/gbfs/provider_town/gbfs"}
testower commented 5 months ago

That's surprising. I can't recall having made a conscious decision to lowercase URLs and I can't find any code path that does it.

testower commented 5 months ago

@hbruch Are you able to work on this?

hbruch commented 5 months ago

This issue can be worked around using lowercase system IDs. I'll look into this after another issue we see currently.

hbruch commented 5 months ago

Seems like FeedUrlUtil#L36 is the culprit lowercasing the system_id.

Looking into the commit history, it seems as it has been like this for a long time already. I suppose, that usually only lowercase system_ids have been used. Would you nevertheless support removing the lowercasing? If yes, I'd prepare a PR.

testower commented 5 months ago

Is it URI.create()? I'm pretty sure String.format with %s preserves the case.

That aside, yes lowercasing was never my intention, feel free to open a PR for this :)

hbruch commented 5 months ago

I'd just remove the toLowercase() call.

testower commented 5 months ago

I'd just remove the toLowercase() call.

Wow, I can't believe I missed that 🗡️ . Well, I guess I must have put it there, but I can't think of any reason why it should be so. So I vote to remove it.

testower commented 5 months ago

Oh wait, the lowercasing is there for the feed names!

testower commented 5 months ago

That must be a remnant of the past, the enum values are already lowercased. Remove it :)