Implement Dynamic Datasource Providers #800
Conversation
# Conflicts: # libs/datamodel/core/src/configuration/source/builtin/README.md # libs/datamodel/core/src/configuration/source/builtin/mod.rs # libs/datamodel/core/src/configuration/source/loader.rs # libs/datamodel/core/src/lib.rs # query-engine/query-engine/src/exec_loader.rs
mavilein
changed the title
mavilein
changed the title
|
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
| pub trait DatasourceProvider { | ||
| /// Passes the provider arg from the datasource. Must return true for all provider names it can handle. | ||
| fn is_provider(&self, provider: &str) -> bool; | ||
|
|
||
| fn canonical_name(&self) -> &str; | ||
|
|
||
| fn can_handle_url(&self, name: &str, url: &StringFromEnvVar) -> Result<(), String>; | ||
|
|
||
| fn connector(&self) -> Box<dyn Connector>; |
There was a problem hiding this comment.
Can we add documentation here?
There was a problem hiding this comment.
What do you want to document in this case? It's returning the connector for this DataSourceProvider. And this is already stated in the signature.
| pub fn set_url(&mut self, url: &str) { | ||
| self.url = StringFromEnvVar { | ||
| from_env_var: None, | ||
| value: url.to_string(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
I don't quite follow what's going on in here; could we add documentation?
There was a problem hiding this comment.
What would you add here? Don't know what to document in this case. I added some documentation to StringFromEnvVar in the followup PR. Check this one out if it helps.
There was a problem hiding this comment.
What would you add here?
Haa, trick question! If I knew I would tell you.
I guess I'm unsure in general how StringFromEnvVar relates to the rest of the system. Judging by name I'm wondering why it's not just a String. Some pointers on what the intent of this construct would be helpful for anyone trying to understand what's going on here.
| )) | ||
| let validated_providers: Vec<_> = all_datasource_providers | ||
| .iter() | ||
| .map(|sd| { |
There was a problem hiding this comment.
What does sd stand for here?
There was a problem hiding this comment.
That stood for SourceDefinition which was the old name for DatasourceProvider. Changing this to provider.
| pub use builtin_datasource_providers::*; | ||
| pub use datasource::*; | ||
| pub use datasource_provider::*; |
There was a problem hiding this comment.
One of the things I've struggled with when onboarding is tracking down where variables come from. It makes it hard to understand how the separate modules relate to each other and create a mental model of the crates. I'm not a fan of switching from explicit imports to globs here.
There was a problem hiding this comment.
Why do you think this is a switch? I kept the same pattern for reexport that was present before as well.
There was a problem hiding this comment.
Why do you think this is a switch?
I might be mistaken, but to me this reads as named imports being replaced with globs. There weren't any globs before, but after this patch there are.
I kept the same pattern for reexport that was present before as well.
I know? Like I mentioned earlier in this thread: this is one of the patterns that makes the codebase hard to navigate. To me it comes off as convenient to author, but it trades away legibility. We don't have to change all instances of this pattern in a single go, but as a baseline I would like us to not introduce more of these and think it's worth bringing up in review.
implementation of prisma/prisma#1487
followup PR to address comments: #863