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

[jormungandr] Manage journey request call without coverage but with object uri #4119

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

kadhikari
Copy link
Contributor

@kadhikari kadhikari commented Sep 29, 2023

For a request on journey to navitia without coverage but with params from or to as coordinate:

  • We get all the authorized coverages for the user: coverages attached to user token + free coverages if user is of type with_free_instances.
  • We keep coverages with shape which includes the coordinate in the request.
  • At last we keep only one coverage which contains from and to coordinates.
  • works well and no call to kraken made.

But when instead of coordinate, if we use object_uri

  • We get all the authorized coverages for the user: coverages attached to user token + free coverages if user is of type with_free_instances.
  • We keep coverages which have the object with object_uri (by calling kraken for all the authorized coverages)
  • At last we keep only one coverage which contains from and to coordinates.
  • Here we make many unnecessary calls to kraken each time.

Here is the FIX when object_uri is present in from or to:

  • We get all the authorized coverages for the user: coverages attached to user token + free coverages if user is of type with_free_instances (as before).
  • Call krakens to get coordinate of the object with object_uri in the request and return the first valide coordinate (most of the time only one call to kraken).
  • Use the function to keep coverages with shape which includes the coordinate of the object.
  • The only overhead when compared to request with coordinate = One call to kraken (cached of course).

For more details: https://navitia.atlassian.net/browse/NAV-2334

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

86.7% 86.7% Coverage
0.0% 0.0% Duplication

Comment on lines +245 to +246
if object_coord:
return self._all_keys_of_coord_in_instances(instances, object_coord.lon, object_coord.lat)
Copy link
Contributor

@pbougue pbougue Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 To me, it looks like 2 problems are gonna rise:

  1. Candidate coverages will sometimes not include coverages that know the stop-id
  • Ex. A: A journey without coverage from Lille to Aix (for a free-user of Navitia).
    Candidates are fr-ne only for Lille, fr-se only for Aix: Navitia won't be able to find a single coverage matching the journey (it is something we are currently able to do).
  • Ex. B: A journey without coverage from Nantes to Lisboa.
    No candidate (from coords) for Lisboa (no pt coverage), so no journey, whereas fr-nw is able to process a journey.
  1. Candidate coverages will sometimes include coverages that don't know the stop-id (they just have coords in their shape)
  • Ex. C: A journey for a "full-access" user from LilleFlandres (stop-id from fr-ne) to LilleEurope (stop-id from fr-ne also):
    From the priorities, fr-npdc will be used first instead of fr-ne, and fr-npdc doesn't know the stop-ids from fr-ne (maybe there is a fallback if journey request fails and it's "just" a perf loss).

As discussed: not sure about the best way to progress considering that (@stifoon may have ideas).

  • Really replacing by stop-coords the stop-ids (probably with a free_radius for the case of Lisboa) when using no coverage
  • Having multiple types of coverage candidates (they may be implicit to save some performance):
    • golden: the ones that know both stop-id and shape includes stop-coord
    • silver: the ones that just know stop-id but shape doesn't include stop-coord
    • bronze: the ones that don't know stop-id, but shape includes stop-coord (this one is probably just a nice-to-have as it would improve current feature and requires switching on stop-coord everywhere in the call
  • Switch to mimir if it does know both coverages and coords for a given stop-id
  • Accept those changes of behavior
  • Change nothing

Copy link
Contributor

@pbougue pbougue Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion, decisions are:

  • assuming the behavioral change on the first problem (examples A & B)
  • watching and testing the second problem (example C):
    • if it doesn't work (no fallback between coverages): reconsider what to do
    • if it still does works: watch if it impacts performances (requesting multiple krakens for a single journey)

@kadhikari kadhikari merged commit 651b7fd into dev Oct 4, 2023
@kadhikari kadhikari deleted the fix_find_coverages_by_id branch October 4, 2023 15:06
@pbougue pbougue changed the title Manage journey request call without coverage but with object uri [jormungandr] Manage journey request call without coverage but with object uri Oct 5, 2023
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.

5 participants