[MAGNOLIA-5154] The population of the Model with request parameters should be optional. Created: 03/Jul/13  Updated: 14/Aug/13  Resolved: 30/Jul/13

Status: Closed
Project: Magnolia
Component/s: rendering
Affects Version/s: 4.5.9, 5.0
Fix Version/s: 4.5.10, 5.0.2, 5.1

Type: Improvement Priority: Critical
Reporter: Philipp Bärfuss Assignee: Roman Kovařík
Resolution: Fixed Votes: 0
Labels: performance, rendering
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: PNG File Screen Shot 2013-07-03 at 11.17.03 PM.png    
Issue Links:
dependency
documentation
to be documented by DOCU-440 Autopopulate rendering model from req... Closed
relation
is related to MAGNOLIA-4726 Array-Style GET Parameters break rend... Closed
is related to MAGNOLIA-5203 The location of rendering properties ... 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)
Date of First Response:

 Description   

In info.magnolia.rendering.renderer.AbstractRenderer.newModel(Class<T>, Node, RenderableDefinition, RenderingModel<?>) we use BeanUtils.populate() which is responsible for 8% of the calculation time.

We should make this configurable on the template definition (with a default setting of false) or see if we can implement something more performant.



 Comments   
Comment by Philipp Bärfuss [ 03/Jul/13 ]

See for the proof.

Comment by Jan Haderka [ 15/Jul/13 ]

Ok, so if I read the changes correctly, only explicitly listed props in the definition of given component will be used to prepopulate model with? And if there is nothing defined, nothing will be prepopulated. Correct? That definitively needs to be explained in the issue and documented at release time. Specially since this is a breaking change. The behavior itself should be made configurable so ppl could go back to old style to prepopulate everything all the time if they don't care about performance and don't want to change their current config.

Additionally. Removing the workaround for beanutils is fine, but I don't see how new code works better then bean utils if it encounters property w/ index e.g. if you had your checkbox prooperty in the test delivered by server as series of checkbox[0], checkbox[1], etc. instead of single array. AFAIK that was the reason for the original code.

Also what i fail to see in the test is proof that this new impl is not slower then the original one. I think there should be a performance test for it.

Comment by Roman Kovařík [ 16/Jul/13 ]

custom populating settings removed because of insignificant performance impact
http://git.magnolia-cms.com/gitweb/?p=magnolia_main.pub.git;a=commitdiff;h=f8382acc2ba76bae8d6ab6b9fee21c4d500da061
autoPopulateFromRequest=true in RenderableDefinition enables populating from request, default is false
http://git.magnolia-cms.com/gitweb/?p=magnolia_main.pub.git;a=commitdiff;h=617e6d83daf4971e7237a67dbdb68f39171492a7

TODO:
create DOCu ticket after review

Comment by Roman Kovařík [ 22/Jul/13 ]

TODO: Set DOCU-440 to Open after review.

Comment by Roman Kovařík [ 24/Jul/13 ]

Autopopulation of models can be set globally:
https://git.magnolia-cms.com/gitweb/?p=magnolia_main.git;a=commitdiff;h=c52b894bba985916cafcf13690bd4bfb502c7dc8
This setting can be overridden on definition:
https://git.magnolia-cms.com/gitweb/?p=magnolia_main.git;a=commitdiff;h=53b59898401a4fb54f119818d69639c55e0496e9

Comment by Jan Haderka [ 26/Jul/13 ]

location of the property is not correct.

Comment by Jan Haderka [ 30/Jul/13 ]

all deprecated tags need to list version since which code is deprecated

  * @deprecated use {@link #FreemarkerRenderer(FreemarkerHelper, RenderingEngine)}

should be

  * @deprecated since 4.5.10, use {@link #FreemarkerRenderer(FreemarkerHelper, RenderingEngine)}
Generated at Mon Feb 12 04:02:30 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.