aem-core-wcm-components icon indicating copy to clipboard operation
aem-core-wcm-components copied to clipboard

[Container] Missing @Exporter annotation - JSON output skipped in model.json Export

Open stefanseifert opened this issue 4 years ago • 7 comments

Fixes #1407

stefanseifert avatar Feb 18 '21 20:02 stefanseifert

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.1% 1.1% Duplication

sonarqubecloud[bot] avatar Feb 18 '21 20:02 sonarqubecloud[bot]

It would be nice to add a unit test to verify that the JSON output of the container component is available.

jckautzmann avatar Feb 19 '21 08:02 jckautzmann

unit tests are already in place. the absence of this annotation does not change the result in unit tests. it would only be possible to test it in integration tests from outside testing the model.json output.

stefanseifert avatar Feb 21 '21 07:02 stefanseifert

I can see this breaking different use cases. Right now, with the missing annotations, the ResponsiveGrid exporter is used (through inheritance chain). We will probably need to ensure that when responsiveGrid layout is used we include the export from the ResponsiveGrid as well.

vladbailescu avatar Feb 24 '21 07:02 vladbailescu

yes, this can definitely break applications relying on the "wrong" output of the ResponsiveGrid model. i do not see a benefit to switch models for simple/responsive layout - the output of the old ResponsiveGrid is really not well suited for JSON export (outputting a lot of authoring-related stuff like allowedComponents which should not be contained in the JSON output).

a pity this annotation was not in place when the container was introduced. probably now this change has to be postponed to version 3.0.0 to no break things, this would be a clean cut.

stefanseifert avatar Feb 24 '21 19:02 stefanseifert

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

sonarqubecloud[bot] avatar Jun 23 '22 12:06 sonarqubecloud[bot]

Codecov Report

Merging #1408 (2945108) into main (f49a734) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1408   +/-   ##
=========================================
  Coverage     86.84%   86.84%           
  Complexity     2433     2433           
=========================================
  Files           216      216           
  Lines          6523     6523           
  Branches       1003     1003           
=========================================
  Hits           5665     5665           
  Misses          342      342           
  Partials        516      516           
Impacted Files Coverage Δ
...onents/internal/models/v1/LayoutContainerImpl.java 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f49a734...2945108. Read the comment docs.

codecov[bot] avatar Jun 23 '22 12:06 codecov[bot]