-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add validation to the serviceJourney Transmodel API endpoint #6559
base: dev-2.x
Are you sure you want to change the base?
Conversation
…point in the Transmodel API to be non-null. It fails fast if not.
…urneys endpoint in the Transmodel API.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6559 +/- ##
=============================================
- Coverage 70.25% 70.24% -0.02%
+ Complexity 18387 18386 -1
=============================================
Files 2088 2088
Lines 77425 77429 +4
Branches 7843 7842 -1
=============================================
- Hits 54395 54389 -6
- Misses 20258 20271 +13
+ Partials 2772 2769 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java
Outdated
Show resolved
Hide resolved
…TransmodelGraphQLSchema.java Co-authored-by: Henrik Abrahamsson <[email protected]>
if (args == null) { | ||
return null; | ||
} | ||
return args.stream().filter(Objects::nonNull).filter(s -> !s.isEmpty()).toList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general it's best to be strict and not have special handling for empty strings and other edge cases. Cleaning user data hides bugs in the clients where they should be fixed.
If a user says they want the service journeys with private codes [ "" ]
then they should get the service journeys that has that private code, which will most probably be an empty set.
What do you think about that?
The null filtering is fine though, we probably need this since we for some reason allowed the elements to be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the opposite of what @vpaturet expressed in #6545 (comment)
I tend to agree with Vincent here, that empty strings should be ignored ie. treated the same as nulls. This also matches how the mapIDsToDomain method handles empty (or even blank strings), using
OpenTripPlanner/utils/src/main/java/org/opentripplanner/utils/lang/StringUtils.java
Line 25 in e8e9a77
public static boolean hasValue(String text) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow response here!
I must say that I disagree with treating empty strings as nulls. If we need to do it for backwards compatability I can see the point, but in this case it looks like we are making the api less strict than previously. The stricter we make the api and the less special cases we add the easier it will be to maintain.
We had a short discussion around this on the developer meeting two weeks ago, and I think the consensus was to avoid special handling for empty strings unless it's needed for compatability reasons.
application/src/main/java/org/opentripplanner/apis/transmodel/support/GqlUtil.java
Outdated
Show resolved
Hide resolved
You have to regenerate schema. |
Summary
This change adds validation to the serviceJourney endpoint in the Transmodel API by requiring that at least one of the argument lists be non-null. It also aligns the meaning of passing an empty list and an empty string as values of the arguments. More details in the issue linked below. This change does not require that there always be at least one active date set, therefore this change does not close the issue.
Issue
#6545
Unit tests
No unit tests in this part of the code. Ran locally to verify that things are working.