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

Mismatch rental_apps OPTIONAL vs CONDITIONALLY REQUIRED #193

Open
1 of 3 tasks
hbruch opened this issue Sep 5, 2024 · 3 comments
Open
1 of 3 tasks

Mismatch rental_apps OPTIONAL vs CONDITIONALLY REQUIRED #193

hbruch opened this issue Sep 5, 2024 · 3 comments

Comments

@hbruch
Copy link

hbruch commented Sep 5, 2024

What is the issue and why is it an issue

GBFSv2.3 as well as GBFS3.0 declare system_information.rental_apps as optional.

In contrast, the gbfs-json-schema instead deems it required in case the feeds contain rental_uris, as @testower pointed out to me in a private conversation. An example can currently be seen here in this example.

Please describe some potential solutions you have considered (even if they aren’t related to GBFS).

I suggest to declare rental_apps, rental_apps.android and rental_apps.ios explicitly CONDITIONALLY REQUIRED in the spec.

Is your potential solution a breaking change?

  • Yes
  • No
  • Unsure

As the gbfs-json-schema is already stricter than the spec, I don't expect serious effects, but others may see this differently.

@testower
Copy link
Collaborator

testower commented Sep 5, 2024

Just for clarity, the json schema doesn't mention this since it can't have cross-file references. But the validator has a rule for it. Presumably because the requiredness property is considered transitive, i.e. it is conditionally required because a sub-field is conditionally required. I'm not sure this is consistent: there could be conditionally required (or required) sub-fields within an optional structure which are not transitive. So I agree with @hbruch better to make this explicit.

@richfab
Copy link
Contributor

richfab commented Sep 13, 2024

Thank you @hbruch and @testower for raising this mismatch between the spec and the validator 🙏

Problem

(as pointed out by @hbruch)

The spec says that the field rental_apps is OPTIONAL in system_information.json (reference). But:

Observed Expected
The validator returns an error saying rental_apps is REQUIRED. The validator should not return any error when rental_apps is not defined in system_information.json.
image image

This error happens only when a field rental_uris is defined in other files of the same feed (in station_information.json or in vehicle_status.json).

As pointed by @testower, it's the validator that adds a custom validation rule here.

Solution

Let's remove this custom validation from the validator since this is not an explicit requirement in the spec currently.

Moving this issue to the validator repo. cc @emmambd

Feel free to open a new issue if you'd like to open a vote to make rental_apps Conditionally REQUIRED when rental_uris is defined in station_information.json or in vehicle_status.json.

@richfab richfab transferred this issue from MobilityData/gbfs Sep 13, 2024
@richfab richfab changed the title Mismatch rental_apps OPTIONAL vs CONDITIONALLY REQUIRED in gbfs-json-schema Mismatch rental_apps OPTIONAL vs CONDITIONALLY REQUIRED Sep 13, 2024
@testower
Copy link
Collaborator

Does it actually make any sense not to provide rental_apps when you have rental_uris?

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

No branches or pull requests

3 participants