[MGNLUI-3402] CompositeFieldDefinition doesnt intialize fieldsName array sometimes Created: 21/Apr/15  Updated: 06/Aug/15  Resolved: 28/Apr/15

Status: Closed
Project: Magnolia UI
Component/s: forms
Affects Version/s: 5.3.8, 5.4
Fix Version/s: 5.3.9

Type: Bug Priority: Neutral
Reporter: Christopher Zimmermann Assignee: Mikaël Geljić
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
dependency
is depended upon by MTE-16 Convert existing components to YAML C... 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)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

If getFieldsName is called before setFields or addFields populates the fields collection, then the fieldsName array will never be populated based on the fields.

This is because getFieldsName sets the fieldsName collection to an empty collection. - so that initFieldsName will not populate it the next time getFieldsName is called, even if setFields does add some fields.

This problem occurs with YAML configuration - as it does call getFieldsName before setFields.



 Comments   
Comment by Christopher Zimmermann [ 21/Apr/15 ]

updateFieldsName method replaces initFieldsName. It is called when addField, setFields and addFieldName is called so that the fieldsName collection is sure to contain an entry for all Node2Bean / Map2Bean calls and direct calls to addFieldName.

Comment by Christopher Zimmermann [ 21/Apr/15 ]

I did discuss the approach with Eric beforehand. the fieldNames must include the names of the fields list - this is the bug that needs to be fixed. I'll ask Eric to take a look too.
Addressing review:
*Changed Map to Set.
*Deprecated getFieldsName and created getFieldNames - and adjusted the callers of this method to use the new method.
*Adjusted test as specified.

Except... I'm not aware of this method: getFieldsNameHasNoSideEffects().

Comment by Christopher Zimmermann [ 22/Apr/15 ]
  • UpdateFields is removed.
  • getFieldNames builds a list of fieldnames 'on demand' from the added fields and the added names.
  • Changed deprecation version to 5.3.9
  • Test updated
Comment by Mikaël Geljić [ 24/Apr/15 ]

Guys,

  1. Didn't this occur to you that this should not be done in the definition at all? Seeing a -DefinitionTest doesn't make your eyes bleed? It should!
    Definitions are—and should remain—stupid.
  2. Afaik it is a bad idea to have out-of-sync fieldNames vs. getters/setters for n2b, it might result in too many and/or inconsistent property descriptors, and be an open door for misconfiguration.
    • Call these fieldNames, getFieldNames(), setFieldNames(), and use consistent type (List is sufficient in that case}
    • Remove/deprecate addFieldName(), add methods are deprecated, see info.magnolia.jcr.node2bean.PropertyTypeDescriptor.getAddMethod()
    • Fill in the blanks according to fields and fieldNames in the CompositeFieldFactory

Cheers,

Comment by Christopher Zimmermann [ 28/Apr/15 ]

Made CompositeFieldDefinition a simple definition and added the fieldName logic to SwitchableFieldFactory.
Deprecated addFieldName, as it does nothing now - and have it report a warning.

https://git.magnolia-cms.com/gitweb/?p=magnolia_ui.git;a=commitdiff;h=9bed9f2765a41371b6d9641ac6042e6f4abee582

Comment by Mikaël Geljić [ 28/Apr/15 ]

Thanks, Reviewed
Minor comments:

  • re: logger, place static fields above regular fields
  • renamed test to behavior-style, makes outcome explicit and serves as complementary spec
    callGetFieldNamesBeforeSetFields() => fieldNamesAndFieldsStayInSynchWhenGetFieldsIsCalledBeforeSetFields

I also took the occasion to fix/improve CompositeFieldDefinition javadoc, and to reorder getter/setter methods for fields.
Will now proceed with integration.

Comment by Mikaël Geljić [ 28/Apr/15 ]

List of fieldNames is obtained via guava transform, which gives a view on the original list—while some transformers (e.g. MailSecurityTransformer) are modifying/clearing that list.
We need to make sure the original fields list isn't modifiable through fieldNames.

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