finos
Add intent with context listener
Describe your change
Adds a new function addIntentWithContextListener to the DesktopAgent interfaces and implements it within DesktopAgentProxy
Related Issue
resolves #1545 recreating #1590 - to be closed
Contributor License Agreement
- [x] I acknowledge that a contributor license agreement is required and that I have one in place or will seek to put one in place ASAP.
Review Checklist
- [x] Issue: If a change was made to the FDC3 Standard, was an issue linked above?
- [x] CHANGELOG: Is a CHANGELOG.md entry included?
- [x] API changes: Does this PR include changes to any of the FDC3 APIs (
DesktopAgent,Channel,PrivateChannel,Listener,Bridging)?- [ ] Docs & Sources: If yes, were both documentation (/docs) and sources updated?
JSDoc comments on interfaces and types should be matched to the main documentation in /docs - [ ] Conformance tests: If yes, are conformance test definitions (/toolbox/fdc3-conformance) still correct and complete?
Conformance test definitions should cover all required aspects of an FDC3 Desktop Agent implementation, which are usually marked with a MUST keyword, and optional features (SHOULD or MAY) where the format of those features is defined - [ ] Schemas: If yes, were changes applied to the Bridging and FDC3 for Web protocol schemas?
The Web Connection protocol and Desktop Agent Communication Protocol schemas must be able to support all necessary aspects of the Desktop Agent API, while Bridging must support those aspects necessary for Desktop Agents to communicate with each other- [ ] If yes, was code generation (
npm run build) run and the results checked in?
Generated code will be found at/src/api/BrowserTypes.tsand/or/src/bridging/BridgingTypes.ts
- [ ] If yes, was code generation (
- [ ] Docs & Sources: If yes, were both documentation (/docs) and sources updated?
- [ ] Context types: Were new Context type schemas created or modified in this PR?
- [ ] Were the field type conventions adhered to?
- [ ] Was the
BaseContextschema applied viaallOf(as it is in existing types)? - [ ] Was a
titleanddescriptionprovided for all properties defined in the schema? - [ ] Was at least one example provided?
- [ ] Was code generation (
npm run build) run and the results checked in?
Generated code will be found at/src/context/ContextTypes.ts
- [ ] Intents: Were new Intents created in this PR?
- [ ] Were the intent name prefixes and other naming conventions & characteristics adhered to?
- [ ] Was the new intent added to the list in the Intents Overview?
THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE FINOS CORPORATE CONTRIBUTOR LICENSE AGREEMENT.
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.
Deploy Preview for fdc3 ready!
| Name | Link |
|---|---|
| Latest commit | e81cfe73e3b2b728843084ec2415f0c87d6e6039 |
| Latest deploy log | https://app.netlify.com/projects/fdc3/deploys/68fb9baf77735c0008ba6185 |
| Deploy Preview | https://deploy-preview-1594.preview-fdc3.finos.org |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify project configuration.
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Coverage Report
Commit: e81cfe7 Base: main@1300f2b
| Type | Base | This PR |
|---|---|---|
| Total Statements Coverage |  97.16% |
97.17% (+0.01%) |
| Total Branches Coverage | 86% |
85.9% (-0.1%) |
| Total Functions Coverage | 96.13% |
96.14% (+0.01%) |
| Total Lines Coverage | 97.32% |
97.33% (+0.01%) |
Details (changed files)
| File | Statements | Branches | Functions | Lines |
|---|---|---|---|---|
| packages/fdc3-agent-proxy/src/DesktopAgentProxy.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/intents/DefaultIntentSupport.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/listeners/DefaultIntentListener.ts | 100% | 81.81% | 100% | 100% |
| packages/fdc3-standard/src/api/DesktopAgent.ts | 100% | 100% | 100% | 100% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts | 98.08% | 89.23% | 100% | 97.84% |
Details (all files)
| File | Statements | Branches | Functions | Lines |
|---|---|---|---|---|
| packages/fdc3-agent-proxy/src/DesktopAgentProxy.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/index.ts | 100% | 100% |  62.5% | 100% |
| packages/fdc3-agent-proxy/src/apps/DefaultAppSupport.ts | 88% | 50% | 100% | 88% |
| packages/fdc3-agent-proxy/src/channels/DefaultChannel.ts | 78.94% | 77.77% | 71.42% | 78.94% |
| packages/fdc3-agent-proxy/src/channels/DefaultChannelSupport.ts | 98.71% | 100% | 94.44% | 100% |
| packages/fdc3-agent-proxy/src/channels/DefaultPrivateChannel.ts | 97.5% | 66.66% | 100% | 97.5% |
| packages/fdc3-agent-proxy/src/heartbeat/DefaultHeartbeatSupport.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/intents/DefaultIntentResolution.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/intents/DefaultIntentSupport.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/listeners/AbstractListener.ts | 100% | 60% | 100% | 100% |
| packages/fdc3-agent-proxy/src/listeners/DefaultContextListener.ts | 100% | 90% | 100% | 100% |
| packages/fdc3-agent-proxy/src/listeners/DefaultIntentListener.ts | 100% | 81.81% | 100% | 100% |
| packages/fdc3-agent-proxy/src/listeners/EventListener.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/listeners/HeartbeatListener.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/listeners/PrivateChannelEventListener.ts | 93.33% | 72.72% | 100% | 93.33% |
| packages/fdc3-agent-proxy/src/messaging/AbstractMessaging.ts | 94.59% | 100% | 80% | 94.59% |
| packages/fdc3-agent-proxy/src/util/AbstractFDC3Logger.ts | 100% | 94.11% | 100% | 100% |
| packages/fdc3-agent-proxy/src/util/Logger.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-agent-proxy/src/util/throwIfUndefined.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-get-agent/src/index.ts | 100% | 100% |  28.57% | 100% |
| packages/fdc3-get-agent/src/messaging/MessagePortMessaging.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-get-agent/src/messaging/message-port.ts | 97.43% | 86.66% | 100% | 97.43% |
| packages/fdc3-get-agent/src/sessionStorage/DesktopAgentDetails.ts | 97.36% | 89.47% | 100% | 97.36% |
| packages/fdc3-get-agent/src/strategies/DesktopAgentPreloadLoader.ts | 100% | 77.77% | 100% | 100% |
| packages/fdc3-get-agent/src/strategies/FailoverHandler.ts | 100% | 76.47% | 100% | 100% |
| packages/fdc3-get-agent/src/strategies/HelloHandler.ts | 94% | 81.25% | 100% | 94% |
| packages/fdc3-get-agent/src/strategies/IdentityValidationHandler.ts | 95.65% | 73.33% | 100% | 95.65% |
| packages/fdc3-get-agent/src/strategies/PostMessageLoader.ts | 98.48% | 86.95% | 100% | 98.46% |
| packages/fdc3-get-agent/src/strategies/Timeouts.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-get-agent/src/strategies/getAgent.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-get-agent/src/ui/AbstractUIComponent.ts | 97.14% | 71.42% | 100% | 97.01% |
| packages/fdc3-get-agent/src/ui/DefaultDesktopAgentChannelSelector.ts | 100% | 75% | 100% | 100% |
| packages/fdc3-get-agent/src/ui/DefaultDesktopAgentIntentResolver.ts | 100% | 90% | 100% | 100% |
| packages/fdc3-get-agent/src/ui/NullChannelSelector.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-get-agent/src/ui/NullIntentResolver.ts | 100% | 100% | 66.66% | 100% |
| packages/fdc3-get-agent/src/util/Logger.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-get-agent/src/util/Uuid.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/index.ts | 91.3% | 70.83% | 60% | 95% |
| packages/fdc3-standard/src/api/AppIdentifier.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/AppIntent.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/AppMetadata.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/Channel.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/ContextMetadata.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/DesktopAgent.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/DisplayMetadata.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/Errors.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/Events.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/GetAgent.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/Icon.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/Image.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/ImplementationMetadata.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/IntentMetadata.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/IntentResolution.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/Listener.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/Methods.ts | 94.18% | 84.28% | 96.29% | 95.23% |
| packages/fdc3-standard/src/api/PrivateChannel.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/RecommendedChannels.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/api/Types.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/context/ContextType.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/intents/Intents.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/internal/contextConfiguration.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/internal/intentConfiguration.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/internal/typeHelpers.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/ui/ChannelSelector.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/ui/Connectable.ts | 100% | 100% | 100% | 100% |
| packages/fdc3-standard/src/ui/IntentResolver.ts | 100% | 100% | 100% | 100% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/BasicFDC3Server.ts | 100% | 100% | 100% | 100% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/ServerContext.ts | 100% | 100% | 100% | 100% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/directory/BasicDirectory.ts | 96.87% | 84.21% | 100% | 96.55% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/BroadcastHandler.ts | 96.38% | 86.41% | 100% | 96.12% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/HeartbeatHandler.ts | 88.23% | 71.87% | 86.66% | 90% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts | 98.08% | 89.23% | 100% | 97.84% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/OpenHandler.ts | 97.14% | 86.84% | 100% | 97.14% |
| toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/support.ts | 100% | 100% | 100% | 100% |
I think this covers most of the functionality but as far as I can see there is still a gap.
In the reference implementation I cannot see how an app that registers an intent with addIntentListener is displayed in the app resolution dialog. I assume that these apps come from the directory but as far as I can see the records in the directory are never updated based on incoming addIntentListener requests.
@robmoffat - can you give me some guidance on how this works?
Converted to draft as docs, unit tests and conformance tests still need to be updated
Hi @Roaders,
To answer your question above, take a look at IntentHandler.ts: it keeps a list of registrations
private registrations: ListenerRegistration[] = [];
when someone adds an intent listener. The directory isn't changed itself - this is static based on what is loaded from the AppD endpoints.
You're going to need to add some tests (see find-intent.feature and raise-intent.feature) for your new method on the server side and also in the proxy side (see find-intents.feature and raise-intents.feature)
HTH?
Hi @Roaders,
To answer your question above, take a look at
IntentHandler.ts: it keeps a list of registrationsprivate registrations: ListenerRegistration[] = [];when someone adds an intent listener. The directory isn't changed itself - this is static based on what is loaded from the AppD endpoints.
You're going to need to add some tests (see
find-intent.featureandraise-intent.feature) for your new method on the server side and also in the proxy side (seefind-intents.featureandraise-intents.feature)HTH?
Yup, I have updated the IntentHandler code to add the contextTypes to the stored registrations. I can't see though how this list of registrations is used to populate the application resolution dialog.
Can you point to the code that gets the list of application to display in the resolution dialog?
Thanks
Can you point to the code that gets the list of application to display in the resolution dialog?
Yeah I can't see it either. Seems like it only looks at what the appD record declares. I'm not sure if this is correct behaviour or not, but It passes the conformance tests so if it isn't we are probably missing some tests.
My feeling is this might be a gap in the spec: the directory interop section should be consistent with the intent listeners that are registered otherwise this is going to cause either errors (when an app says it's going to register an intent listener but doesn't) or inconsistent behaviour (the app registers an unexpected intent listener, meaning the intents will get resolved differently depending on which apps are running). @kriswest?
I thought about a conformance test but it would have to be a manual one as the user would have to select the registered app in the app resolver for the test to pass... I guess... Unless we make sure that only one app is registered I suppose.
@Roaders @robmoffat its upto the desktop agent to return the options for further resolution. Thats handled here in the proxy: https://github.com/finos/FDC3/blob/63eceaa9d1f1790c0c6d0f463b010965a187dd07/packages/fdc3-agent-proxy/src/intents/DefaultIntentSupport.ts#L159-L165
The DA has sent back app intents for all resolution options, which are then passed to the (injected) intent resolver. If the DA showed its won resolver then it doesn't return the appIntent as its already resolved, so this path is only for injected resolvers.
In the reference implementation intent resolution happens in this block (depending on whether and how a target was set): https://github.com/finos/FDC3/blob/63eceaa9d1f1790c0c6d0f463b010965a187dd07/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts#L455-L474
The reference implementation doesn't currently support dynamic registration AFAIK (and should be updated to do so) as the resolution always relates to the app directory, that happens here :
https://github.com/finos/FDC3/blob/63eceaa9d1f1790c0c6d0f463b010965a187dd07/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts#L384-L385
This block is where it will resolve if no target was set and if it only found one option: https://github.com/finos/FDC3/blob/63eceaa9d1f1790c0c6d0f463b010965a187dd07/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts#L407-L423
Then finally here it returns options to show in the resolver if multiple options were found: https://github.com/finos/FDC3/blob/63eceaa9d1f1790c0c6d0f463b010965a187dd07/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts#L424-L439
As you guys are finding this hard to navigate perhaps we need to add some more comments to it? It does take a while to read and interpret that code...
I thought about a conformance test but it would have to be a manual one as the user would have to select the registered app in the app resolver for the test to pass... I guess... Unless we make sure that only one app is registered I suppose.
A manual test is worth adding.
However, I did just have a random thought about injecting a resolver just for testing that can do the selection... but then not all DAs will use injection for the resolver so that might be too specific.
Seems like it only looks at what the appD record declares. I'm not sure if this is correct behaviour or not, but It passes the conformance tests so if it isn't we are probably missing some tests.
Dynamic registration is currently optional (MAY) so we don't test for conformance on it.
My feeling is this might be a gap in the spec: the directory interop section should be consistent with the intent listeners that are registered otherwise this is going to cause either errors (when an app says it's going to register an intent listener but doesn't)
We have a specified error (ResolveError.IntentDeliveryFailed) and minimum timeout allowed for registration in the spec (but no maximum timeout as one could not be agreed):
https://github.com/finos/FDC3/blob/63eceaa9d1f1790c0c6d0f463b010965a187dd07/website/docs/api/spec.md?plain=1#L536
or inconsistent behaviour (the app registers an unexpected intent listener, meaning the intents will get resolved differently depending on which apps are running). @kriswest?
...and thats optional support for dynamic registration. Its all covered in the spec (no gaps) but its also not entirely standardized as dynamic registration is optional (MAY).
Does that answer all questions raised in this thread?
Thanks very much for the detailed (as always) replies @kriswest and @robmoffat . Not everything is clear to me yet though I am afraid. To summarise what's been said so far:
- The refernce implementation does not support dynamic registration (so if we add an intent listener it will not appear in the app resolution dialog)
- We should add support for dynamic registration to the reference implementation
@kriswest I am a bit confused regarding adding a conformace test. You replied to my comment saying that we should add a conformance test to ensure that if an intent lisstener is registered that it appears in the resolution dialog but you also replied to Rob to say that dynamic registration is optional so we don't need a conformace test.
It seems to me that we don't need a conformance test as this is an optional feature.
I will add to the Directory so that the IntentHandler can inform it of dynamic registrations
@Roaders
@kriswest I am a bit confused regarding adding a conformace test. You replied to my comment saying that we should add a conformance test to ensure that if an intent lisstener is registered that it appears in the resolution dialog but you also replied to Rob to say that dynamic registration is optional so we don't need a conformace test.
It seems to me that we don't need a conformance test as this is an optional feature.
We do not desperately need a test if its not a required feature - although there is an argument that if an optional feature (MAY OR SHOULD) is present it MUST be conformant to the spec...
However, I made the comment in the context of this, perhaps, becoming a required feature. If we accept the addition of the new API call it should be a MUST (we don't have any other optional API calls AFAIK). Then we should consider whether dynamic registration should be required of a Desktop Agent or not. If it's NOT supported, then the standard is somewhat deficient on advice as to how to handle that (so I misspoke above - that is a gap!). There's no suitable error to return so most such implementations probably just accept the registration but never route anything to it/offer it during resolution. Is that the best solution? If so we should document that.
Hence, I think we have something that should be added to the spec whether we require dynamic registration or not.
To summarise what's been said so far:
- The refernce implementation does not support dynamic registration (so if we add an intent listener it will not appear in the app resolution dialog)
- We should add support for dynamic registration to the reference implementation
💯
The reference implementation now seems to correctly add dynamically registered apps to the app resolver.
I want to update the workbench to allow us to test addIntentWithContextListener. I did have some issues with the workbench when testing this and raise #1612
I was always expecting to do tha twork as part of this PR. Should I not be?
Hi @Roaders,
I think this might be crossing the streams a bit: that last part is a bug-fix that we should really have in the 2.2 branch, whereas the rest of this PR is for FDC3 2.3, I think
Hi @Roaders,
I think this might be crossing the streams a bit: that last part is a bug-fix that we should really have in the 2.2 branch, whereas the rest of this PR is for FDC3 2.3, I think
Yeah, that makes sense. I can cherry pick and raise a PR with just that fix - unless you've already done it?
remaining items that need looking at:
- add tests for DesktopAgentProxy
- DOCUMENTATION (probably need to speak to @kriswest about this)
- clarification of dynamic registration in docs
- add new add intent section to workbench
