[MGNLUI-3882] Reference fields by name as well as by fully qualified classname Created: 18/May/16  Updated: 25/Jun/18  Resolved: 16/May/18

Status: Closed
Project: Magnolia UI
Component/s: None
Affects Version/s: 5.4.6
Fix Version/s: 5.7

Type: Story Priority: Neutral
Reporter: Christopher Zimmermann Assignee: Ilgun Ilgun
Resolution: Done Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLUI-4135 Enable reference-fields-by-name Closed
relates to MAGNOLIA-6928 TypeResolvers do not report any probl... Accepted
dependency
depends upon MAGNOLIA-6901 Generalise bean type resolution strat... Closed
depends upon MGNLUI-4377 Report references to deprecated field... Closed
depends upon MGNLUI-4380 Report references to deprecated field... Closed
depends upon MGNLUI-4427 Disambiguating duplicate field-types Closed
is depended upon by MAGNOLIA-6664 Inheritance of registry definitions f... Closed
is depended upon by MGNLUI-4496 Infer field definition type from 'typ... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Release notes required:
Yes
Documentation update required:
Yes
Date of First Response:
Epic Link: LD: Simple dialog config
Sprint: Basel 79, Basel 147
Story Points: 13

 Description   

Currently in a dialog definition, to specify the type of field you must supply a fully qualified class name of the field, for example:

info.magnolia.ui.form.field.definition.TextFieldDefinition

For legibility, ease of development, and to make fields easier to remember, a developer should be able to use a short name instead of needing to specify the class name. (But the fully qualified class name may still be used.) In this case:

text

Still to decide is which field should be used to supply this field name, maybe the same field "class" or a new field "fieldType".

A developer should not have to specify a different dialog class for this functionality - it should work with the current default dialog.

Validation: https://wiki.magnolia-cms.com/display/PMTEAM/Simplified+Dialogs+LDV



 Comments   
Comment by Christopher Zimmermann [ 23/May/16 ]

Idea: Whether to only address this for fieldTypes with a special mechanism, or whether to introduce this ability more generically - the ability to reference a class by a name.
Could their be some kind of IOC for configuration? A separate place where devs can map which class implements a specific name for a field - or even for other things like templates?

Comment by Christopher Zimmermann [ 22/Nov/16 ]

TODO:

  • POC
  • Arch Review
Comment by Aleksandr Pchelintcev [ 07/Dec/16 ]

After thinking about this for some time, I started questioning the necessity of connecting the definition class to an alias via the registry itself. There's a couple of reasons for that:

  • there's no guarantee that all the registries are available during to-bean transformation (so what do we do anyway?)
  • the only current use case of e.g. FieldTypeRegistry is to resolve the field factory class for a field definition. At the moment we do that - alias is redundant: we already know the definition class (cause we have the definition instance).

Another point against attempting to wire up such mechanism over the registry is the support for other similar entities (actions/traits/columns/you name it). If we implement smth for the field defs and it works nicely - users will logically expect us to spread the pattern application. But many (actually most) of other objects aren't covered by the registries at all.

With all that in mind - I start to think that the variant with class annotating is not such a bad idea if done smartly:

  • We could introduce a notion of a type alias that could be configured by either a class/interface-level annotation (@TypeAlias) or via module descriptor XML tag (if Java modifications are not desirable).
  • Aliases could be aggregated in a special component (AliasResolver/AliasLookup/...). Such component would be system-wide and could be accessible from all the data converters and from any other place later on.
  • Aliases could be aggregated via different channels
    • Reflections library is one of them, but we need to scan the whole classpath (own packages + the user packages as well);
    • Compile time annotation processor (might need some research and might be tricky to implement/configure);
    • While parsing module descriptors.
  • Some sort of namespaces would be required (in order to avoid conflicts and be able to achieve a better structure in general).
    • Base class could be automatically used as an implicit namespace
    • Module names somehow?
  • Similarly to how we treat the class property during transformation - we could introduce the property ~type which would indicate that the actual class should be looked up via the alias resolver (property name choice might be tricky since you can't tell, which ones are already specified as actual definition interface fields).
Comment by Ilgun Ilgun [ 07/Dec/16 ]

Actually when I started really looking into N2B and M2B and how to implement it via Registry approach, I had some doubts about it.

If we really need to implement the Registry approach;

  • Registries are not accessible from core anyways. (circular dependency)
  • Map2Bean do not offer custom transformer (can be implemented as far as I see)
  • Custom transformer impl for Node2Bean
  • Since registries are not accessible we have to choose from:
    • Component in core for field alias <-> definition class (FieldTypeRegistry in core essentially)
    • Or moving the FieldTypeRegistry to core (which is no go)

When I offered Annotation based approach, I had more or less similar ideas as Sasha. One important remark from him make sense to me as well regarding module descriptor xml tag.

I'd suggest to have module name in front of the alias e.g fooModule.text
As I already mentioned in the POC I'd use it with @Field(name="text") annotation. I can edit the concept with further ideas and suggestions regarding annotation based approach if that's the way to go.

Comment by Jan Haderka [ 07/Dec/16 ]

While it's good to behave like a boy scout and leave place you have been cleaner than it was when you came, i'm afraid we are getting fairly far away from the original scope of allowing any John Doe to write his definition w/ just name of the field in mind and not remembering full class name.

Also while annotations and effectively hardcoding name of the field in the class would be most likely be no issue for us, remembering some cases in the past, it would be issue for users of 3rd party developed modules that might have same name definitions or where module A introduces field AX that module B wants to replace w/ BX ... that with annotations would probably require us to define dependencies on the modules if that would be even enough.

How about another round of discussion on next Arch meeting before going too deep?

Comment by Ilgun Ilgun [ 07/Dec/16 ]

If it comes to there we have different options and what I would suggest it to not allow same name fields aliases. If one wants to give an alias to a field, we can clearly suggest them to use moduleName prefix. anyhow that's very deep topic at this stage.
On the other hand, It's for sure that we have to communicate this to the user via something (perhaps an app or perhaps a subapp in definitions app )

Comment by Aleksandr Pchelintcev [ 08/Dec/16 ]

had re: problem with the clashing names - anticipating that too, I have outlined in my comment that the effort would require a namespace concept of some sort (implicit/explicit/semi-explicit). Allowing users to provide alias mappings within the module names would make the substitution possible.

Comment by Aleksandr Pchelintcev [ 08/Dec/16 ]

hadilgun We have discussed the matter again at the arch meeting and the conclusion was to hold on with the effort for now mostly due to the high amount of uncertainty about this feature.

  • Initial idea to wire this through the field type registry and have it done in fast and non-intrusive way got complicated by the question of how to make M2B and N2B aware of such mappings?
    • Letting transformers pull registries during transformation process is at least questionable.
    • Registries are downstream relatively to transformers, so this would make a somewhat circular dependency.
    • We would get a cross-registry dependency (during a dialog definition resolution you would require that the field type registry is complete enough to provide necessary info)
  • Proposals like the above which argue for the additional mappings, annotations and new mechanisms imply the introduction of additional abstractions that look tad too heavy for such a small feature.
    • Additional tooling would also be required (e.g. like another sub-app in defs app that would reveal the mappings)
    • One of the ideas for cutting some corners here was to look at the TypeMapping and see if it could be utilised for such purposes.
  • There is a proposal for looking closer at the transformers we have and attempt to bring them to a more open and extensible common API:
    • E.g. allow to register custom resolvers/plugins (still not very clear to me, where the info would be coming from).

As I said in the beginning, it looks like it makes sense to park the effort now and see how the situation evolves and whether this feature could be implemented as a requirement or a complement to some bigger story .

Comment by Jan Haderka [ 08/Dec/16 ]

due to the high amount of uncertainty about this feature

What is uncertainty?
Ever since we moved types to registries, people were crying to be able to specify field types just by name instead of having to remember and copy whole class name of the definition. What is point of having named definitions when we don't use the name? Looks like rather flawed design to begin with.

Be sure that this request is not going away. It will not automagically disappear so I don't understand what is uncertain about it. What do you need to remove the uncertainty? The above statement for sure doesn't make that clear.

-Perhaps I'm just venting my frustration here, but seems to me like lately conclusion to every problem is "let's ground it and see if it doesn't get resolved by itself later".-

Why is resolving the type from dialog field itself, currently you specify in dialog class that is picked by transformer to instantiate the definition. When class property is committed we use the default class for type expected at given position or fail to instantiate bean. We could simply use again type property that we used in the past for this and have default impl be proxy class that would look up the underlying type definition implementation in the registry. The proxy itself could live where registries are or even more downstream to avoid circular dependency.

And I'm sure there are other even better solutions, all I wanted to say is that there is no "later" in "we shall revisit this later". Users struggling with writing solutions on top of Magnolia is the bigger story we are solving now, and having to remember, type correctly or paste class names again and again is part of this bigger story.

Comment by Aleksandr Pchelintcev [ 13/Dec/16 ]

After another iteration of this feature review at the architecture meeting the following decision have been made:

  • We want to have a working solution for the fields that would lookup the definition class based on the name coming from the FieldTypeDefinitionRegistry through metadata (i.e. eventually => node/file names) without the need to provide any redundant aliases.
    • we need to make sure that we leave a room for other strategies as well (if we ever want to have the same mechanism for e.g. actions, which aren't bound to a registry).
    • we need to find a way to gracefully provide the access to FieldTypeDefinitionRegistry from Node2Bean and Map2Bean transformers (through some plugin-like abstractions or similar)

Two concepts have been considered:

Next steps:

  • We ensure that the field type definitions are loaded prior to the dialog/app defs (change the order of config source bootstraps in UiFrameworkModule), so that the latter can safely reference the former.
  • ilgun proceeds with building the production-quality solution based on findings done in PoC, mgeljic and apchelintcev help with review and share the knowledge.
Comment by Bradley Andersen [ 14/Dec/16 ]

I have not read all of this, but my initial thought would be just attack the fieldTypes registry. Then a clash might happen and need to be dealt with if you have created one with the same name in $your_module.

Edit: I have only just learned this ticket exists, and am very happy it has been selected.

Comment by Mikaël Geljić [ 20/Dec/16 ]

Notes from architects meeting:

  • Passing problemTracker as param of #lookupImplementationType?
    • ideally ProblemTracker should be injected
      • then would need to be bound to a transformation context/scope
    • —would-be no-go in the current state of things
    • keep it as is with ProblemTracker /state
  • Cannot compose resolvers, can you? —that's good
  • How do you limit how a resolver qualifies to a bean type?
    • Calls for a #supportsType / #qualifiesFor method
  • Show the policy deciding part?
    • need more deterministic way to find resolver than randomly iterating over unordered set
    • need the fallback to ClassLookup, always
    • —is MultiBinding the proper way to provide such ordering?
    • —do we need custom sorting to guarantee ClassLookup is "last"? (most of the time single one actually)
    • —or more simple alternative/admitted limitation? (haven't figured out how quite yet)
  • Let's pass base interface along to resolver as well
  • Naming => TypeResolver
    • first description was litterally the policy "resolves" bla bla bla
  • Still new type resolution mechanism
    • debated the obscure extension point via "policy" implementation hanging in the air (just bound at runtime on Guice level)
    • —can live with that, general agreement to keep the feature for internal use too
Comment by Christopher Zimmermann [ 05/Jan/17 ]

Please include me in QA.

Comment by Philip Mundt [ 18/Jan/17 ]

See https://jira.magnolia-cms.com/browse/MAGNOLIA-6901?focusedCommentId=137749&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-137749

Generated at Mon Feb 12 09:11:03 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.