To add a little wrinkle and small clarification here, there are some connection types that expose and use both classic Extra as well as customized fields in the connection form (Snowflake being the main example I can think of). As of 2.2, these two Extra options are being merged but not necessarily as a "smart" merge. If both are provided, the Extra JSON simply combines the classic Extra and custom field values.
On Sun, Nov 21, 2021 at 1:30 PM Jarek Potiuk <[email protected]> wrote: > Also - why the MUST: > > If you use custom fields, in the current implementation, when you have > both short and long form "available", when you edit such a connection, > the "extras" dictionary is overridden with long names (simply because > the JSON extra field is disabled and there is no "merge" logic for > handling both. > > So there is a scenario where you have a "short" name defined in extra > initially - but when you edit the connection, it will disappear > (unless you manually fill the "form" field with the same value). > > That's why for connections that implement "custom" field behaviours > the "short" form is really "deprecated" value (to support a case where > the user uses connection_uri or has a database configured with "short" > names from earlier manual "editing" the JSON field). > > And yes - it's not ideal, but I think the only proper way to handle it > is to implement better behaviour of the UI and support for > "namespaced" per-connection short names (which should not be too > difficult for anyone with some flair for javascript + Flask App > Builder). > Until then, we have to live with the dichotomy, I am afraid. > > J. > > On Sun, Nov 21, 2021 at 7:18 PM Jarek Potiuk <[email protected]> wrote: > > > > The problem is that if we use short extras, until we fix that in the > > UI, customizations won't work or they might override each other for > > different connections. > > > > So my take on it is - (until we fix the UI): > > > > a) if the connection customizes UI fields (so that you DO NOT edit the > > extras as JSON in the UI "extras" field manually) - you MUST use the > > long form. > > b) If the connection does not customize UI fields (so that you have to > > edit extras as JSIN in the UI "extras" field manually) - it is > > preferred to use short form > > > > J. > > > > On Sun, Nov 21, 2021 at 7:09 PM Daniel Standish <[email protected]> > wrote: > > > > > > I wasn't exactly proposing updating the front end to handle short > names. It sounds like a reasonable idea but I am not trying to take that > on right now. > > > > > > What I was trying to ask was, given the present form behavior, what > should the hook behavior be, by convention? > > > > > > Specifically, should we write our conn parsing in hooks such that > whenever there is a form extra (e.g. extra__google__hello) we always do > `extra.get('hello') or extra.get('extra__google__hello')` > > > > > > Or something along those lines. > > > > > > Or should we stick with the status quo, which is to say there is no > such convention and some hooks will accept both and others will not. > > > > > > I think my vote would be that by convention we should check both and > short beats long in precedence. This doesn't mean we have to go update > every hook with form customizations. But if we're reviewing a PR that > checks both then it's ok to approve, and if we're reviewing a PR that only > checks long name we can suggest checking short name also. > > > > > > WDYT? > > > > > > >
