search

finos / FDC3

An open standard for the financial desktop.
https://fdc3.finos.org
Other
194 stars 115 forks source link

The `AppIntent` interface incorrectly describes an intent #312

Closed ggeorgievx closed 1 year ago

ggeorgievx commented 3 years ago

Minor Issue

The AppIntent interface incorrectly describes an intent. The displayName of the AppIntent's intent is ambiguous.

Area of Issue

[ ] App Directory [X] API [ ] Context Data [X] Intents [ ] Use Cases [ ] Other

Issue Description:

Let's say we have the following FDC3 application definitions (this is just an example to illustrate the issue - some required properties are missing):

[
    {
        "name": "ChartA",
        "intents": [
            {
                "name": "fdc3.ViewChart",
                "displayName": "Chart A's View Chart Intent",
                "contexts": [
                    "fdc3.instrument"
                ]
            }
        ]
    },
    {
        "name": "ChartB",
        "intents": [
            {
                "name": "fdc3.ViewChart",
                "displayName": "Chart B's View Chart Intent",
                "contexts": [
                    "fdc3.instrument"
                ]
            }
        ]
    }
]

After calling fdc3.findIntent('fdc3.ViewChart') we would get the following AppIntent:

{
    intent: {
        name: 'fdc3.ViewChart',
        displayName: '...'
    },
    apps: [
        {
            name: 'ChartA'
        },
        {
            name: 'ChartB'
        }
    ]
}

The issue is that the display name is ambiguous - it could be Chart A's View Chart Intent or Chart B's View Chart Intent.

Going through the existing open-source FDC3 implementations we noticed that both Nick Kolba's desktop-agent, as well as Finsemble's FDC3 implementation would return an AppIntent with the intent display name of the first application.

Additional Context:

A PR for this issue would require a change to the existing AppIntent interface.

rikoe commented 3 years ago

This is a good catch, we need to fix this, and 2.0 will be a good place to do this, as it may require breaking changes to the AppIntent interface.

kriswest commented 3 years ago

Should we just drop the displayName field from IntentMetadata (which leaves it as a rather pointless class containing only a single string name) ? After all intents should be:

We could then also drop displayName from the appD intents object.

An alternative would be to add some form of reference for intents (other than the config of individual apps), perhaps as part of the appD, where the display name could be drawn from.

kriswest commented 2 years ago

@ggeorgievx any thoughts on how to resolve this issue? I can see how this flaw might have come about:

When Citi showed us their AppD portal, it included a reference for both intents and context types. Perhaps we should go that route, add it as a new record type to the appD and remove the displayName ?config?? Perhaps with a suggested fail-over to the raw intent name?

ggeorgievx commented 2 years ago

IMO the cheapest solution would have us add an optional displayName property to the AppMetadata interface similarly to how we dealt with application instances in #509.

Another solution would be to change the AppIntent interface's apps property to a singular from (app). That way we would have the AppIntent interface describe a single instance/implementation of an intent with the correct metadata like the displayName (and would truly be an AppIntent as opposed to an AppsIntent). We would then also need to change the signature of findIntent() (and possibly the name to findIntent**s**()) to return a promise resolving with an array of AppIntent (Promise<AppIntent[]>).

Thoughts?

rikoe commented 2 years ago

I personally like this suggestion @ggeorgievx - I think it is the "best" way to solve the problem, and the most flexible for the future. An AppIntent represents the relationship between a single app and intent, and you could have multiple of them returned...

ggeorgievx commented 2 years ago

@kriswest @mattjamieson Team, what are your thoughts on this one? If you agree with Riko I am more than happy to raise a PR.

Alternatively we could have this added to the mtg agenda for the next Standards WG.

kriswest commented 2 years ago

I agree that this change makes sense. The semantics of AppIntent have always bothered me as has the hack you have to use to pick a display name for intents.

The affected functions are just these two, right?

  // intents
  findIntent(intent: string, context?: Context): Promise<AppIntent>;
  findIntentsByContext(context: Context): Promise<Array<AppIntent>>;

We could/should switch both to return Promise<Array<AppIntent>>, with each entry then corresponding to single app - there can be a mix of intents in the array in the findIntentsByContext response, but I'd be inclined to be consistent over the return type rather than goto a 2d array (particularly as every entry in the response to findIntent could have a different displayName even when the intents are all the same).

The (breaking) change will of course need approval by the SWG, but I think it's worth implementing the PR to present to the group. Don't forget that it will also need fixing in the Methods.ts.

@thorsent are you also happy with this change?

P.S. apologies for the delay in responding @ggeorgievx, I missed the original notification.

kriswest commented 2 years ago

One more thing to consider... Rex suggested this in the deprecation policy (see #532):

Rather than quietly change the behavior of an existing feature, consider deprecating it and replacing it with something with a different name/syntax.

This is a breaking change without a prior deprecation. Should we consider revising the function and type names and preserve the old ones as deprecated? Implementing a fallback/conversion in Methods.ts is possible.

Not sure what I would call the replacement for AppIntent... IntentApp? Implies findIntentApp and findIntentAppByContext

thorsent commented 2 years ago

@kriswest I like the change.

I think it's worth discussing how to deal with breaking changes more elegantly. Perhaps namespacing offers a more discrete approach:

//Maintain separate namespaces for each official version
export namespace FDC3_1 {
    export type FDC3 = {
        findIntentsByContext(context: string): Promise<AppIntent>
    }
}
export namespace FDC3_2 {
    export type FDC3 = {
        findIntentsByContext(context: string): Promise<Array<AppIntent>>
    }
}

// Default to the most recent version 
export type FDC3 = FDC3_2.FDC3 & {
    fdc3_1 ?: FDC3_1.FDC3, // But provide access to prior versions - if the desktop agent supports it.
    fdc3_2 ?: FDC3_2.FDC3
}

Since fdc3 is a declared global though, this would force developers to be a little more creative:

myapp.ts

import { FDC3_1 } from "@types/@finos/fdc3";

let myFDC3 : FDC3_1 = fdc3.fdc3_1;

const appIntent = await myFDC3.findIntentsByContext("chart");
kriswest commented 2 years ago

@thorsent @ggeorgievx @rikoe For the System channels -> User channels rename, where functions changed name (e.g. getSystemChannels() -> getUserChannels()) we implemented an automated fallback in the NPM Module's Methods.ts. There's another for the addContextListener signature that was deprecated here.

Not everyone uses that NPM module, but it does provide an alternative to namespacing that we could use... It does work best if we change the name of the function and types when making breaking changes (as its easy to detect that the new function is not present and we need to adapt).

Finally, my assumption is that it will usually be easier to update the NPM module in an application than it will be to update the desktop container (which might only provide FDC3 1.2, where your app has been updated to use FDC3 2.0).

thorsent commented 2 years ago

The fallback works fine for JS (and desktop agents could also offer backward compatibility).

The only issue is that existing TS implementations would break during the compile cycle when the published types change. So I think it depends on whether that is an acceptable compromise.

kriswest commented 2 years ago

On returning to this issue, I am less sure about the proposed change (allowing for a display name per intent/app combo). There are situations (e.g. a button to raise an intent in an app, which can then be resolved by multiple other apps) and API calls (findIntentsByContext and raiseIntentForContext and resolver responses/UI) where we need a single display name for an intent. There is a use-case for data unique to an intent/app combo, but its perhaps descriptive rather than nominative (what precisely will the app do when it receive the roughly descriptive intent).

The /intents appD endpoint proposed in #719 also doesn't provide a solution where multiple appDs are in use as you could still end up with multiple display names per intent. We could apply the display names in the standard itself; there exists a source file with display names although I think these are rarely used and not necessarily correct, e.g.:

    {
      "name": "ViewInstrument",
      "displayName": "Details"
    },
    {
      "name": "ViewAnalysis",
      "displayName": "Analyze"
    },
    {
      "name": "ViewHoldings",
      "displayName": "Holdings"
    }

It also doesn't provide a solution for proprietary or as yet unstandardized intents.

Hence, I am again leaning towards simply dropping the display name, in favour of just using the intent name (and living with PascalCase) as that provides a simple solution for all cases.

kriswest commented 2 years ago

Notes from #802: