[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: |
|
||||||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||||||
| 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 ] |
| 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 ] |
|
TODO: |
| Comment by Roman Kovařík [ 22/Jul/13 ] |
|
TODO: Set |
| Comment by Roman Kovařík [ 24/Jul/13 ] |
|
Autopopulation of models can be set globally: |
| 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)}
|