bahn.de boards

[dabund24]

resolves #3

Some notes/uncertainties:

• For the stop property, there is no way to access anything apart from the id.
  • To simplify parsing, I allowed passing a string (the id) to parseLocation(). Other options I am aware of would have been the creation of the station object within parseArrOrDep(), which would not have been too elegant and passing the whole object to parseLocation(), which I didn't really like either, as there is a higher risk of accidentally parsing completely unrelated values.
  • This also leads some integration tests to fail, since the station fptf validator requires the name of a station to exist. How should we deal with this? Is setting this property to an empty string an option?
• I also added a vias option users can set the amount of displayed vias with. I am aware of the stopovers option from hafas-client, but decided against using it for two reasons: stopovers served the purpose of returning the next/previous stopovers as station objects, but here, we only get the names, so the expected functionality would not have been satisfied. Also, by letting vias be a number instead of just a bool, we can expose the maxVias query from the api to users

[traines-source]

Thanks, this is looking great already!

However, we definitely need to preserve the regio-guide endpoint for the db profile (among others since it's still the only endpoint that supports querying more than 1 hour). In #3, I had proposed to introduce a new profile, e.g. dbweb where one could move all implementations of the bahn.de endpoints. If you have another idea, that's also fine.

For the stop/station parsing, I'm fine with all three options, however, as I've commented below, the station enrichment should still work, i.e. you should not return early from parseLocation. One of the other two options probably makes this easier (and whole, unrelated objects are passed in many places, even though I agree it's ugly :) This could also resolve the int test issue, if you set enrichStations: true in db.js#L85 (or rather, in dbweb.js or sth) (but this may lead to race conditions, since the station index is loaded asynchronously, so maybe we will need another solution, not sure).

Regarding vias, one could also employ enrichStations for that (based on name matching), but this is probably quite ugly. So while I'm trying to avoid any divergence from hafas-client, I think in this case it is fine how you did it.

[dabund24]

Thanks for the thorough review!

In #3, I had proposed to introduce a new profile, e.g. dbweb where one could move all implementations of the bahn.de endpoints. If you have another idea, that's also fine.

Long term, this is probably the best idea. I'll attempt to implement that

For the stop/station parsing, I'm fine with all three options, however, as I've commented below, the station enrichment should still work, i.e. you should not return early from parseLocation. One of the other two options probably makes this easier (and whole, unrelated objects are passed in many places, even though I agree it's ugly :)

I wasn't aware of station enrichment at all and will toy around with it

This could also resolve the int test issue, if you set enrichStations: true in db.js#L85 (or rather, in dbweb.js or sth) (but this may lead to race conditions, since the station index is loaded asynchronously, so maybe we will need another solution, not sure).

A rather naive question, but why would race conditions be an issue here? Are there multiple stations with the same station index?

Regarding vias, one could also employ enrichStations for that (based on name matching), but this is probably quite ugly. So while I'm trying to avoid any divergence from hafas-client, I think in this case it is fine how you did it.

I'll try to see how well things work with name matching. If it turns out to work well, I'd be in favor of doing it the dirty way if that helps making this library a better drop in replacement of hafas-client 🤔

[traines-source]

Long term, this is probably the best idea. I'll attempt to implement that

If you don't want to invest the time, I can also have a look at it.

A rather naive question, but why would race conditions be an issue here? Are there multiple stations with the same station index?

Maybe race conditions is the wrong term. Tests might be toggling, because sometimes, the station index will have loaded in time before the respective tests execute and sometimes the test will execute first, making them fail (since station details are missing). But I haven't actually tested it, maybe it just works.

If it turns out to work well, I'd be in favor of doing it the dirty way

Yup sounds good, the guys from this issue would surely be happy about it I guess.
The index would then need to be built for names as well here, similar to what is already done for fallback locations based on the ctxRecon here and here.

[dabund24]

If you don't want to invest the time, I can also have a look at it.

I'd be happy to try that myself. Thanks for the offer!

The index would then need to be built for names as well here, similar to what is already done for fallback locations based on the ctxRecon here and here.

Thanks for the useful hints 🙏

[dabund24]

What I tried doesn't seem to work for older node versions, hold on...

[traines-source]

Haven't looked at it in detail, what's the reason you're doing this dynamic stuff? Can't you just import stuff from other profiles and the default profile in db/index.js? (Although dynamic switching between profiles would be great, if that's what it does.)

[dabund24]

The idea would be, that we have a single source of truth (dynamicProfileData.json in this case) for which profile endpoint uses which profile. Otherwise, if we wanted to change the preferred profile, we'd have to ensure that the imports are consistent with the base urls. Here's a longer explanation I have prepared:

This adds the dbregioguide and dbweb profiles and adds a new db profile, which is supposed to combine the best of all worlds as suggested in #3. It works like this:
In dynamicProfileData.json, we can specify which profile should be used for which client endpoint. If we wanted to change the preferred profile for a client endpoint, the only thing to change would be a single line in there (the corresponding profileName value).
In db/index.js, all necessary functions and endpoint bases are imported dynamically based on the json file. If a function does not exist, nothing is imported and the function from the default profile is used instead.
If we want to decide at runtime, which profile to use, we can set the profile name to db and add the needed module(s) to the db profile, where the call is delegated to a different profile based on some conditions (see trip-req.js for instance).
Both the new and old unit tests and e2e tests all pass (edit: they don't) (modulo the ones, where station enrichment provides a fix), so it seems to work fine. The only downside here is, that the code in db/index.js is rather hard to read, but I couldn't find a different solution apart from manually importing all files and additionally manually adding the corresponding bases, which would lead to less maintainable code.

However, apparently, older node versions have a problem with synchronously importing an es module with require. As the language standard specified in .eslintrc.json is still at es2021, there is also not the possibility of using await import(), as top-level awaits are only supported from es2022 onwards, so there may be no way to import something dynamically and synchronously anyways 🤔. So just importing the stuff explicitly may be the only solution anyways, idk

[traines-source]

What you could do is just import everything in db/index.js with profile prefixes and then just dynamically put the correct/desired (already imported) functions etc. into the db profile. That should definitely work.

But I'm currently not sure whether this setup actually simplifies anything, although I agree that without it one does have to make sure that the base urls match the imports (but one could also import/reuse the base urls from the profiles and at least don't have to hardcode the urls twice, i.e. still not needing a base.json for db). Also, I don't think this should change all too often, since it is in some sense a breaking change for the consumers (rather, we might introduce even more alternative "hybrid" profiles down the line).

What would be interesting is to be able to switch to another profile on the fly (e.g. by passing a parameter to the REST API). However, I don't really see that the dynamicProfileData.json approach brings us closer to that, because it doesn't solve the problem of the global profile in createClient, which you have nicely circumvented in trip-req.js (such that, as far as I can tell, concurrent requests are still possible).

(Thinking about it, it may be best to implement on-the-fly profile switching downstream e.g. in db-rest and just instantiate multiple createClient instances. The overhead should be minimal, apart from the station index, which one could easily refactor to optionally pass from the outside, in order to avoid it being loaded in every instance. – But all of that is not really related to this PR.)

Apart from that, the splitting into dbweb and dbregioguide is looking good! Not sure how to proceed. What do you think?

[dabund24]

Thinking about it, I will resort to the simplest option of statically importing just the necessary stuff and putting the functions/endpoint bases into the profile one by one. If something goes wrong here, this should be caught by the db integration tests anyways

[dabund24]

I think this should be good. I'll implement the rest of this pr later today

[dabund24]

The changes from 73a71e0 to the dbweb arrival fixture show that removing all non-digits greatly improves the results ^^

[dabund24]

A rather naive question, but why would race conditions be an issue here? Are there multiple stations with the same station index?

Maybe race conditions is the wrong term. Tests might be toggling, because sometimes, the station index will have loaded in time before the respective tests execute and sometimes the test will execute first, making them fail (since station details are missing). But I haven't actually tested it, maybe it just works.

That was indeed a problem. Right now I make the tests repeatedly call the corresponding board function every four seconds until the station objects can be enriched. Otherwise, the test just timeouts. This is unbelievably ugly, but I couldn't come up with a better solution. Can you think of a better solution here?

[dabund24]

The code for adding the stopovers in parseArrOrDep() is stolen pretty much one by one from hafas-client. Tomorrow, I will replace the dbweb-departures test. Apart from some more minor fixes based on your feedback, we are done then, I believe

[traines-source]

Yes so if we decide switch over db from bahn.de to DB Navigator (see comment), we should definitely verify again that not too much stuff changes (as far as I see, there is no test/fixture anymore that does that rn).
For the enrichStations test hack, one could benefit from being able to optionally pass the station index into the client (idea in the context of on-the-fly profile switching). For the time being, I think we can leave it like you have done it and replace it as soon as we've implemented this, which I intend to do soon anyways.

[dabund24]

I think this is it. Let me know if you notice some more issues

[traines-source]

Cool, thanks a lot! Will hopefully come around to create a release tomorrow (that is, today).