Yeah. We really need to clean it up - but I am afraid without fixing the UI for managing the connections in a sane way, the status quo (even if it's not very clear how it works) will remain.
J. On Mon, Nov 22, 2021 at 6:00 AM Josh Fell <[email protected]> wrote: > > 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? >> > > >> > >
