Support for x-provided-by Pull pattern #567
Conversation
| ) | ||
| .unwrap(); | ||
| } | ||
| fn register_method( |
There was a problem hiding this comment.
Registers firebolt method with jsonrpsee, using the callback indicated by Self::callback_*() depending on the type of method that's being registered.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Various callback types follow (callback_*()), these are effectively executed when the associated firebolt API call is made.
| async fn callback_provider_invoker( | ||
| params: Params<'static>, | ||
| context: Arc<RpcModuleContext>, | ||
| ) -> Result<Value, Error> { |
There was a problem hiding this comment.
I'm not too sure about this return type (Value). If we want to avoid mapping response data to specific types this needs to return a Value so the receiver of the response can deserialize it as it knows the type. But does this clash with the way the firebolt schema is currently defined? Not sure how we want to handle this.
There was a problem hiding this comment.
Feel this could be a Value as long as we evaluate using the OpenRpc Validator for Request/Response and Event. Making Ripple an effective Pass through with Schema validation support,
There was a problem hiding this comment.
I think calls like Content.requestUserInterest expect the actual struct to be returned, e.g. in this case EntityDetails and not Value::Object(EntityDetails), is that right @kpears201 ?
There was a problem hiding this comment.
...and if that's the case how will we ever get away from having to map response types like we still do in ProviderRegistrar::get_provider_reponse() (here)?
There was a problem hiding this comment.
Another point, for responses from APIs that have x-provided-by, ripple needs to be able to set the app ID parameter to the value of the app ID parameter (if it exists) in the provider response (RPPL-2310), meaning ripple needs to be able to process the response and not just pass-through.
| Ok(None) | ||
| } | ||
|
|
||
| pub fn register_methods(platform_state: &PlatformState, methods: &mut Methods) -> u32 { |
There was a problem hiding this comment.
Iterates through the provider map and registers firebolt methods by type.
| @@ -49,6 +49,7 @@ impl GenericCapState { | |||
| FireboltCap::Short("token:platform".to_owned()), | |||
| FireboltCap::Short("usergrant:acknowledgechallenge".to_owned()), | |||
| FireboltCap::Short("usergrant:pinchallenge".to_owned()), | |||
| // <pca> Do we need to add "discovery:interest" etc. here or just in the manifests? </pca> | |||
There was a problem hiding this comment.
I'm not sure why these are hardcoded here, I think when GenericCapState::new() is called the manifest has already been ingested so what's the point, why not just include these in the manifest?
There was a problem hiding this comment.
When Ripple starts up these capabilities are set to unavailable. So any calls for these providers would fail with unavailable error.
Once the providers are registered the broker would then set these capabilities as available. From that point the consecutive calls would pass
There was a problem hiding this comment.
@satlead: Right but these get set as a result of BootstrapState::build() being called, which is before the providers are registered. Because of that they might as well just be defined in the manifest right?
There was a problem hiding this comment.
@rdkcentral/ripple-qa please note
|
|
…so calls like Content.onUserInterest can be made before provider is launched.
Minimum allowed line rate is |
What
What does this PR add or remove?
Why
Why are these changes needed?
How
How do these changes achieve the goal?
Test
How has this been tested? How can a reviewer test it?
Checklist