Change rbx_reflection_database to support loading from FS #376
Conversation
Specifically, databases are now loaded from a local directory or from an environmental variable if either exist.
rbx_reflection_database/src/lib.rs
Outdated
| log::debug!("Loading local reflection database from {}", location.display()); | ||
| Some( | ||
| rmp_serde::decode::from_slice(&file).unwrap_or_else(|e| { | ||
| panic!("could not decode reflection database because: {}", e) |
There was a problem hiding this comment.
How hard would it be for the user to fix broken reflection?:
- it's not in the easiest place to get to
- tools that download reflection may break if they depend on this crate first (because panic), making it hard to fix the reflection database automatically
We don't need to fix this now, but we should probably fix it once using the local database becomes the norm.
We should ensure this is easy to change to a recoverable error in the future. Maybe in the future this can be a Result and get can return the bundled database if the local database is an Err.
There was a problem hiding this comment.
I did actually consider just using a a Result rather than panicking for exactly this reason. It's not an unfounded worry because we've seen problems with a file exactly like this for Selene.
The reason I didn't commit to it out of the gate was because I didn't want to force the get to suddenly be a fallible operation, but I suppose it actually is now and lying isn't doing anyone favors. I'm willing to commit to it now so we only have to break things once.
rbx_reflection_database/src/lib.rs
Outdated
| /// | ||
| /// Panics if the file specified by `RBX_DATABASE` or in the default location | ||
| /// exists but is invalid MessagePack. | ||
| pub fn get_local() -> Option<&'static ReflectionDatabase<'static>> { |
There was a problem hiding this comment.
We should be careful about this return type if we want to make invalid local databases a recoverable error in the future.
Some options:
- We leave this as it is and we have a breaking change in the future to return a Result
- We leave this as it is and we return None in the future if deserializing failed. We can add another method to check for decoding failure vs missing database if needed.
- We make this private so we don't have any API contract to uphold.
- We commit to making invalid local database a panic for the foreseeable future, making automatic recovery difficult to implement.
|
Allowing the database to be replaced once it's initialized is complicated because it involves mutating a static variable, so for the time being I'm going to ignore it. Tools like Rojo and Lune can work out solutions without that. |
As part of the effort to get the reflection database loadable locally, it's probably a good idea to have a central place that decides locations for the entire ecosystem. This will prevent consumers like Lune and Rojo from having to re-implement this logic.
The obvious choice for this, in my opinion, is rbx_reflection_database. It's a requirement for everything that uses the reflection database already, and its public API can be extended to take things from the file system into account without any issues. I've implemented it and documented it thoroughly in the README for rbx_reflection_database.
However, it also sacrifices the 'purity' of the module because it fundamentally alters its purpose. I'm okay with that, but I understand not everyone will be. Please be honest with your thoughts.
I know for sure this works on Windows and Linux. I'm assuming it will also work on MacOS, but I don't own a machine I can use for testing. I don't know how to test this in CI, so we'll just have to be mindful of changes.