Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stefanseifert
Copy link
Contributor

Fixes #1407

@sonarcloud
Copy link

sonarcloud bot commented Feb 18, 2021

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

@jckautzmann
Copy link
Contributor

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

@stefanseifert
Copy link
Contributor Author

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.

@vladbailescu
Copy link
Member

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.

@stefanseifert
Copy link
Contributor Author

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.

@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2022

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

@codecov
Copy link

codecov bot commented Jun 23, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Container] Create a Container v2 with the correct JSON output
3 participants