[MAGNOLIA-6306] Ability to export YAML files based on existing JCR configuration Created: 21/Apr/15  Updated: 27/May/16  Resolved: 22/Jul/15

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: 5.4
Fix Version/s: 5.4.1

Type: Improvement Priority: Neutral
Reporter: Christopher Zimmermann Assignee: Ilgun Ilgun
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: 0d
Time Spent: 5d 6h 25m
Original Estimate: 1d 7h

Issue Links:
dependency
depends upon MAGNOLIA-6322 JcrConfigurationSource may produce in... Closed
relation
is related to MGNLUI-3492 Add Action for yaml export in Configu... 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:
Epic Link: Config by file / code
Sprint: Sprint 2 (Basel)
Story Points: 5

 Description   

There should be an easy mechanism to export YAML files based on existing JCR configuration:

  • Some developers will have projects they want to convert to YAML.
  • Developers may test or prototype things in Config app - and then want to export them.

Notes:
A logical location for the feature would be an action in the Configuration app.
An alternative would be a new app or a new feature in the Exports app.



 Comments   
Comment by Aleksandr Pchelintcev [ 20/Jul/15 ]

ExportYamlComand

  • Overall
    • Would be nice to export a RawView (or some generic interface) <-> Yaml processor object in order not to mix the command logic and actual export
  • Name should reflect the following:
    • the command actually exports JCR node into Yaml
    • not every node can be exported (only ones that correspond to defs)
  • JavaDoc: ditto
    • explain what can be exported and how/under which circumstances
  • setAndGetRepository/setAndGetPath - unfortunate method names
    • also I'd demand that the precise workspace and path are set since the defaults are not appropriate anyway
    • there's no config to be exported to yaml in 'webste' w/s (which is default)
    • converting the root into Yaml won't work and that's generally a bad idea
  • The following is not necessary
    /code Collection<Registry> all = registryFacade.all();
    Preconditions.checkArgument(all != null && !all.isEmpty(), "Registry should not be null or empty");
    • it is a workaround for missing functionality and should be encapsulated in definitionProviderFor
    • a comment in the method mentioning a ticket to be fixed, so that thesearch isn't that brute - would be nice
  • processRawView:
    • iteration over the sub-beans has wrong iterator var name (nestedCollection -> nestedBean?)
  • tests:
    • create the sample files instead of the large non-yaml-looking strs
Comment by Aleksandr Pchelintcev [ 22/Jul/15 ]

Went thoroughly through implementation. Some findings:

ExportJcrNodeToYamlCommand:

  • JavaDoc was barely improved since last discussion - would you understand what it tries to say as an outside user?
  • We extracted DefinitionRawViewToYamlConverter interface to be able to inject it, yet in command the impl object is directly instantiated.
  • Big 'try' block should be scoped to minimum - to operations with resources only.
  • static MgnlContext methods should be avoided unless absolutely no other way. Here the Context object is provided as method argument. In generic case - you can inject Context. Dependency on MgnlContext object causes quite severe architecture problems and we try to avoid it atm.

DefinitionRawViewToYamlConverterImpl:

  • Why all the methods including the interface method "throws RepositoryException"!?
    • Component has nothing to do with JCR.
  • Name of the interface method should be not "export" but "convert"
  • Slipped away from me initially - but what is SnakeYaml used here for?
    • the #dump() method is to be used with plain Java objects (like maps) - here you already provide Yaml
    • sanitizeYaml() method can go away as well
  • The algorithm prints unnecessary tab in the beginning (before - it was mangled by Snake Yaml sanitising).
  • Methods with 4-5 arguments are really hard to analyse:
    • e.g. after spending some time I managed to find out that:
      • processCollectionItem#isCollection argument is redundant
      • so is the method isOneLiner() - its result never affects anything after all.
  • Conclusion: if you need some state to be passed on between several method calls - create a state object and use it instead of a bunch of method arguments.

DefinitionRawViewToYamlConverterImplTest:

  • ByteArrayOutputStream outputStream - not used
  • test method names and variables aren't adapted to the test object naming changes.

Pls take a look at review/refactor/MAGNOLIA-6306-apch branch which contains fixes for the aforementioned issues.

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