-
Notifications
You must be signed in to change notification settings - Fork 1
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
LPD-42482 Rework data set serialization I (URL and actions) #4669
base: master
Are you sure you want to change the base?
Conversation
CI is automatically triggering the following test suites:
|
ci:test:relevant |
❌ ci:test:sf - 0 out of 1 jobs passed in 34 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-37531_LPD-42482 1 Failed Jobs:For more details click here.format-source-files: [java] Loading file:/opt/dev/projects/github/liferay-portal/portal-impl/classes/system.properties [java] java.lang.Exception: Found 3 formatting issues: [java] 1: Incorrect commit message in SHA 9dd12a4d06994066177a38200ede11d50962589d: Each breaking change should have one, and only one "# breaking", "## What", "## Why" and ## (Optional). Use "----" to split each breaking change.: ./modules/apps/frontend-data-set/frontend-data-set-api/bnd.bnd (SourceCheck:BNDBreakingChangeCommitMessageCheck) [java] 2: Incorrect commit message in SHA 9dd12a4d06994066177a38200ede11d50962589d: The commit message contains "# breaking" should end with "\n\n----": ./modules/apps/frontend-data-set/frontend-data-set-api/bnd.bnd (SourceCheck:BNDBreakingChangeCommitMessageCheck) [java] 3: ./modules/apps/frontend-data-set/frontend-data-set-api/src/main/java/com/liferay/frontend/data/set/action/FDSItemActionList.java expected:<...pyrightText: (c) 202[4] Liferay, Inc. https...> but was:<...pyrightText: (c) 202[5] Liferay, Inc. https...> [java] [java] at com.liferay.source.formatter.SourceFormatter.format(SourceFormatter.java:472) [java] at com.liferay.source.formatter.SourceFormatter.main(SourceFormatter.java:301) [stopwatch] [run.batch.test.action: 31:44.952 sec] [echo] The following error occurred while executing this line: [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:449: The following error occurred while executing this line: [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:651: Java returned: 1 [delete] Deleting: /opt/dev/projects/github/liferay-portal/null1438725624.properties |
Jenkins Build:test-portal-source-format#7262 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#4669 Testray Routine:EE Pull Request Testray Build ID:111032910 Testray Importer:publish-testray-report#27637 |
Jenkins Build:test-portal-acceptance-pullrequest(master)#12219 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4669 Testray Routine:EE Pull Request Testray Build ID:111081566 Testray Importer:publish-testray-report#26767 |
24755a9
to
200111d
Compare
ci:test:sf |
200111d
to
817de3d
Compare
ci:test:sf |
ci:test:relevant |
…ata will implement it
…sets, with unit tests
… data sets, with unit tests # breaking ## What modules/apps/frontend-data-set/frontend-data-set-api/src/main/java/com/liferay/frontend/data/set/action/FDSCreationMenu.java deleted method * public CreationMenu getCreationMenu(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse) throws PortalException; added method * public CreationMenu getCreationMenu(HttpServletRequest httpServletRequest) ## Why Response is not necessary to compute the creation menu. Also, we want implementations to handle their exceptions and return an empty menu if something goes wrong, so that the serialization does not stop. This is a recently added interface, not yet implemented. Customers are not affected at the time of this writing ----
…tem data sets, with unit tests
…ts, with unit tests. Custom data sets don't yet support definition of bulk actions so we provide a placeholder implementation to facilitate reference handling in the renderer
812aea6
to
67de3a9
Compare
ci:test:sf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsanz please see comments inline
HttpServletRequest httpServletRequest, | ||
HttpServletResponse httpServletResponse) | ||
throws PortalException; | ||
public CreationMenu getCreationMenu(HttpServletRequest httpServletRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will conflict with my actions import task. It's no problem, just a bit of extra work for one of us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm homogeneizing this because I had to artificially add the httpServletResponse
parameter in the PoC, then we never removed it. Request shall be enough to calculate this
HttpServletRequest httpServletRequest, | ||
HttpServletResponse httpServletResponse) | ||
throws PortalException; | ||
HttpServletRequest httpServletRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also conflict
/** | ||
* @author Daniel Sanz | ||
*/ | ||
public class FDSTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this will be acceptable. Recently, because of brianchandotcom#157046 (comment) I needed to change FDSTimeZoneBehaviors
into FDSTimeZoneBehaviorConstants
. This might need to be FDSTypeConstants
or FDSConstants
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, this naming is improvable
* @author Daniel Sanz | ||
*/ | ||
@Component( | ||
property = "dataset.type=" + FDSTypes.CUSTOM, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name of the property does not seem right. Maybe fds.entry.type
? To be consistent with constants, FDSTypes
should probably be FDSEntryTypeConstants
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a bit more comfortable with dataset
here because it is detached from the storage mechanism. fdsEntry
sounds to me as biased towards java-based stuff.
Nevertheless, I understand consistency might be more important here, so I'm changing it.
@Override | ||
public CreationMenu serialize( | ||
String fdsName, HttpServletRequest httpServletRequest) { | ||
|
||
CreationMenu creationMenu = new CreationMenu(); | ||
|
||
for (ObjectEntry objectEntry : | ||
_customFDSSerializerHelper.getCreationActionObjectEntries( | ||
fdsName, httpServletRequest)) { | ||
|
||
creationMenu.addPrimaryDropdownItem( | ||
dropdownItem -> { | ||
Map<String, Object> properties = | ||
objectEntry.getProperties(); | ||
|
||
dropdownItem.putData( | ||
"disableHeader", | ||
(boolean)Validator.isNull(properties.get("title"))); | ||
|
||
dropdownItem.putData( | ||
"permissionKey", properties.get("permissionKey")); | ||
dropdownItem.putData("size", properties.get("modalSize")); | ||
dropdownItem.putData("title", properties.get("title")); | ||
|
||
dropdownItem.setHref(properties.get("url")); | ||
dropdownItem.setIcon( | ||
String.valueOf(properties.get("icon"))); | ||
dropdownItem.setLabel( | ||
String.valueOf(properties.get("label"))); | ||
dropdownItem.setTarget( | ||
String.valueOf(properties.get("target"))); | ||
}); | ||
} | ||
|
||
return creationMenu; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, this is not really a serialization. Serialization is transformation of complex data structure into a string, or a similar simple format, some linear series of bytes. Here, we are transforming one type of object (ObjectEntry
) into another (CreationMenu
). But, something like FDSCreationMenuTransformer
also doesn't sound right to me. I would expect this type of functionality to be a method rather than a class, or a part of constructor. Something like
CreationMenu creationMenu = new CreationMenu(creationActionObjectEntries);
or
CreationMenu creationMenu = SomeHelperUtil.getCreationMenu(creationActionObjectEntries);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an important thing about naming.
Original, pre-existing serializers are FDSViewSerializer
and FDSFilterSerializer
which basically return a JSONArray
from the list of java objects representing views and filters.
Originally, I planned to do a similar thing for the list of actions (creation menu and friends).
Nevertheless, CreationMenu
happens to extend HashMap, one of the valid things we put on the props map that gets serialized for react render.
In light of the above, it makes no sense to convert creation menu related data into something different than CreationMenu
class. This is why the dedicated serializer class extends the generic one parameterizing T
with CreationMenu
directly:
public interface FDSCreationMenuSerializer extends FDSSerializer<CreationMenu> {}
From this reasoning, implementations of FDSCreationMenuSerializer
for custom and system data sets do directly provide an instance of CreationMenu
All the above sounds reasonable to me, except naming. Because as you said, this is not exactly serialization but more like "conversion". Even, for the case of SystemFDSCreationMenuSerializer
, implementation just fetches the object from the registry so there is even no conversion at all.
I'm open to other names, as long as we consider all the variants we have.
} | ||
|
||
private FDSAPIURLBuilder _addNestedFields( | ||
FDSAPIURLBuilder fdsapiurlBuilder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fdsAPIURLBuilder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me recheck the SF
System.arraycopy( | ||
fieldNameList, 0, fieldsName, 0, fieldNameList.length - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without going too deep into what's going on here, there is probably a nicer way to achieve it, a util or a tweak when making an array instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the existing code in our fragment, so I guess we can consider this later on
❌ ci:test:sf - 0 out of 1 jobs passed in 31 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-37531_LPD-42482 1 Failed Jobs:For more details click here.format-source-files: [java] Loading file:/opt/dev/projects/github/liferay-portal/portal-impl/classes/system.properties [java] java.lang.Exception: Found 7 formatting issues: [java] 1: ./modules/third-party/jenkins-core/build.gradle expected:<...", version: "3.0.1" [java] [ [java] ] compileOnly group: ...> but was:<...", version: "3.0.1" [java] [] compileOnly group: ...> [java] 2: ./modules/third-party/org-apache-axis/build.gradle expected:<...", version: "1.6.3" [java] [ [java] ] compileOnly group: ...> but was:<...", version: "1.6.3" [java] [] compileOnly group: ...> [java] 3: ./modules/third-party/org-outerj-daisy-diff/build.gradle expected:<...ff", version: "1.2" [java] [ [java] ] compileOnly group: ...> but was:<...ff", version: "1.2" [java] [] compileOnly group: ...> [java] 4: ./modules/third-party/org-hibernate-core-3.6.10/build.gradle expected:<...sion: "3.2.0.Final" [java] [ [java] ] compileOnly group: ...> but was:<...sion: "3.2.0.Final" [java] [] compileOnly group: ...> [java] 5: ./modules/third-party/org-opensearch-java-client/build.gradle expected:<... version: "2.25.53" [java] [ [java] ] compileOnly group: ...> but was:<... version: "2.25.53" [java] [] compileOnly group: ...> [java] 6: ./modules/third-party/org-drools-core/build.gradle expected:<...sion: "5.4.0.Final" [java] [ [java] ] compileOnly group: ...> but was:<...sion: "5.4.0.Final" [java] [] compileOnly group: ...> [java] 7: ./modules/third-party/org-opensearch-rest-client/build.gradle expected:<..., version: "4.4.15" [java] [ [java] ] compileOnly group: ...> but was:<..., version: "4.4.15" [java] [] compileOnly group: ...> [java] [java] at com.liferay.source.formatter.SourceFormatter.format(SourceFormatter.java:472) [java] at com.liferay.source.formatter.SourceFormatter.main(SourceFormatter.java:301) [stopwatch] [run.batch.test.action: 28:20.136 sec] [echo] The following error occurred while executing this line: [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:449: The following error occurred while executing this line: [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:651: Java returned: 1 [delete] Deleting: /opt/dev/projects/github/liferay-portal/null1991035310.properties |
Jenkins Build:test-portal-source-format#3210 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#4669 Testray Routine:EE Pull Request Testray Build ID:113094676 Testray Importer:publish-testray-report#13249 |
✔️ ci:test:stable - 24 out of 24 jobs passed✔️ ci:test:relevant - 31 out of 31 jobs passed in 1 hour 18 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 09cc2b52634eb72cdebf54b7aac9afe4ba545311 ci:test:stable - 24 out of 24 jobs PASSED24 Successful Jobs:
ci:test:relevant - 31 out of 31 jobs PASSED31 Successful Jobs:
For more details click here.Test bundle downloads: |
Jenkins Build:test-portal-acceptance-pullrequest(master)#10779 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4669 Testray Routine:EE Pull Request Testray Build ID:113111302 Testray Importer:publish-testray-report#25541 |
last sf runs returns unrelated failures, probably due to some change in the CI. |
ci:test:sf |
❌ ci:test:sf - 0 out of 1 jobs passed in 37 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-37531_LPD-42482 1 Failed Jobs:For more details click here.format-source-files: [java] Loading file:/opt/dev/projects/github/liferay-portal/portal-impl/classes/system.properties [java] java.lang.Exception: Found 7 formatting issues: [java] 1: ./modules/third-party/org-apache-axis/build.gradle expected:<...", version: "1.6.3" [java] [ [java] ] compileOnly group: ...> but was:<...", version: "1.6.3" [java] [] compileOnly group: ...> [java] 2: ./modules/third-party/org-hibernate-core-3.6.10/build.gradle expected:<...sion: "3.2.0.Final" [java] [ [java] ] compileOnly group: ...> but was:<...sion: "3.2.0.Final" [java] [] compileOnly group: ...> [java] 3: ./modules/third-party/org-drools-core/build.gradle expected:<...sion: "5.4.0.Final" [java] [ [java] ] compileOnly group: ...> but was:<...sion: "5.4.0.Final" [java] [] compileOnly group: ...> [java] 4: ./modules/third-party/org-opensearch-java-client/build.gradle expected:<... version: "2.25.53" [java] [ [java] ] compileOnly group: ...> but was:<... version: "2.25.53" [java] [] compileOnly group: ...> [java] 5: ./modules/third-party/org-opensearch-rest-client/build.gradle expected:<..., version: "4.4.15" [java] [ [java] ] compileOnly group: ...> but was:<..., version: "4.4.15" [java] [] compileOnly group: ...> [java] 6: ./modules/third-party/jenkins-core/build.gradle expected:<...", version: "3.0.1" [java] [ [java] ] compileOnly group: ...> but was:<...", version: "3.0.1" [java] [] compileOnly group: ...> [java] 7: ./modules/third-party/org-outerj-daisy-diff/build.gradle expected:<...ff", version: "1.2" [java] [ [java] ] compileOnly group: ...> but was:<...ff", version: "1.2" [java] [] compileOnly group: ...> [java] [java] at com.liferay.source.formatter.SourceFormatter.format(SourceFormatter.java:472) [java] at com.liferay.source.formatter.SourceFormatter.main(SourceFormatter.java:301) [stopwatch] [run.batch.test.action: 34:25.524 sec] [echo] The following error occurred while executing this line: [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:449: The following error occurred while executing this line: [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:651: Java returned: 1 [delete] Deleting: /opt/dev/projects/github/liferay-portal/null420668434.properties |
Jenkins Build:test-portal-source-format#7677 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#4669 Testray Routine:EE Pull Request Testray Build ID:113157722 Testray Importer:publish-testray-report#5423 |
Reference: https://liferay.atlassian.net/browse/LPD-42482
Parent task: https://liferay.atlassian.net/browse/LPD-42481
Once both java interfaces necessary to describe a data set (https://liferay.atlassian.net/browse/LPD-44141) and FDS API URL builder (https://liferay.atlassian.net/browse/LPD-44140) are in place, we can homogeneize seralization.
This is the first of 2 PRs where we will add some components in charge of serializing different parts of the Data Set. Main features of this proposal are:
Serializer
name has been inspired on the existingFDSFilterSerializer
andFDSViewSerializer
classes, which will be integrated into this new architecture in the second PRThis first PR handles serialization of URLs and actions (item, creation menu and bulk)
Additional notes: