-
Notifications
You must be signed in to change notification settings - Fork 684
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
SOLR-14414: Introduce new UI (SIP-7) #2605
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for sharing this POC @malliaridis , this looks awesome!
A few high-level observations:
- Not that I understand every aspect of this PR perfectly, but I find this code much easier to comprehend than the existing Admin UI Angular code. If there was a particular UI element I wanted to tweak, I feel like I'd already have an easier time finding the underlying 'Card'/'Component' in the source code than doing the corresponding tracing in the current JS app. (Admittedly, that could be an unfair comparison since there's a lot more in the Angular UI at this point. But it's as good of a comparison as we're going to get, so 🤷 )
- The biggest downside in the code for me is how boilerplate heavy it is. There are Component interfaces, which have Component implementations, which then reference Models, which come from Stores, which fetch the data via API calls. Those abstractions all make sense individually. But they're the main reason probably that this turned into an 8k line PR!
Positing community consensus on taking this forward, I think we'll need to break this into smaller pieces that are less intimidating to folks. I suspect you've already thought a bit about where to draw some of those lines, but I have at least some hunches we could talk over at some point. (Split "Java Properties" and "logging" pages into different PRs, slim the theme down to only support a single mode, commit the theme separately, etc.) - Also pleasantly surprised that there weren't a ton of changes needed to existing code. Only a few minor edits to LoadAdminUiServlet, jetty.xml, app.js
I've left a few comments inline. Please treat those all as questions only at this point. There's a few places I flagged things that are likely to need changes, but no need to handle those while we're still in POC mode, unless you particularly want to. Just wanted to make sure they didn't get lost.
gradle/libs.versions.toml
Outdated
@@ -0,0 +1,65 @@ | |||
[versions] |
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.
[Q] The rest of our build puts library versions in the versions.props
and versions.lock
files in the repo root, which are read and enforced by a gradle plugin (palantir:consistent-versions
, iirc) to ensure we're not pulling in many versions of the same dep.
Is the separate version-management scheme here just a part of the POC, or do you think this is something we'd need to retain in the eventual non-POC version of this PR as well?
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.
[0] Not necessary at this point, but leaving a comment here as a reminder that eventually we'll need to license-check all of these deps, if this logic doesn't get integrated with versions.props/versions.lock (see comment above)
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 am currently working on a migration to use version catalogs in the entire project (outside this POC). I will probably discuss version catalogs soon in the mailing list.
I saw it in the Lucene project and I am using version catalogs on personal projects too, so it was the easiest and quickest way to get started. But it should definitely be either-or, not both present like in this POC. See apache/lucene#13484.
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/icons/SolrLogo.kt
Outdated
Show resolved
Hide resolved
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/main/MainContent.kt
Show resolved
Hide resolved
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/navigation/MainMenu.kt
Show resolved
Hide resolved
* critical errors or warnings. | ||
*/ | ||
|
||
val primaryLight = Color(0xFF9C1F00) |
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.
[Q] Are these values all things you created by hand, or was there something that generated them? Did they come from a template somewhere?
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.
There are many tools that can generate these values. They are coming from the Material 3 UI components. Material 3 is one of the UI libraries that can be used and is common in Android apps and Google products.
Here I used Figma to export the colors / theme I used in the designs, bu that didn't work as expected. You will see some differenes in the colors if you compare it with the Figma designs. So the values would need adjustment to match the designs in Figma.
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/utils/HttpClientUtils.kt
Show resolved
Hide resolved
Also - when I check out this code locally it compiles fine in my IDE, but I'm having trouble running the packaging tasks (e.g.
Going to try this on different machines and hopefully work around it. But if anyone has any insights (or even specific commands that worked for them), I'd love to get this working locally to kick the tires of the new UI. |
solr/server/etc/jetty.xml
Outdated
@@ -95,7 +95,7 @@ | |||
<New class="org.eclipse.jetty.rewrite.handler.HeaderPatternRule"> | |||
<Set name="pattern">/solr/*</Set> | |||
<Set name="name">Content-Security-Policy</Set> | |||
<Set name="value">default-src 'none'; base-uri 'none'; connect-src 'self'; form-action 'self'; font-src 'self'; frame-ancestors 'none'; img-src 'self' data:; media-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self'; worker-src 'self';</Set> | |||
<Set name="value">default-src 'none'; base-uri 'none'; connect-src 'self'; form-action 'self'; font-src 'self'; frame-ancestors 'none'; img-src 'self' data:; media-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'wasm-unsafe-eval'; worker-src 'self';</Set> |
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.
wasm
! Fun! I heart that tech. ;-)
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.
Good thing you pointed this line out. This is one of the security "considerations" that are necessary when introducing a wasm application to our current UI.
You are right. There are multiple layers that abstract API, logic and UI from each other, resulting in a lot of boilerplate code. The repetitiveness is also a side-effect of applying the same patterns to all componets. Besides the drawback of the quantity in changes, there are two main benefits that may be achieved with that approach: testing components / individual parts becomes much easier, and new developers have an easier start because they can follow the structure and patterns from other components when they are working on a new component. I wanted to write some tests for completion, but I had to postpone it for the time being.
You are completely right on that. It is possible, and this was the plan once we work on the Jira part of the migration, to split the components into separate PRs and tickets. This would allow in the long run an easier integration of new components. Contributors will also have to worry only about a single component and not the entire application when working on a ticket, PR, feature or bugfix. The POC includes already two of the components for demonstration, so that it covers almost all parts of the Compose app (including navigation) that are crucial to get started. The only thing I can recall at the moment and that is missing is probably the explicit error handling, but that is something I have already worked on in other projects and shouldn't be a problem integrating.
Thank you very much for your time reviewing this. As you noticed, this POC still needs some work and cleanup before moving forward. |
I am getting an error when running
Going to try and change that property! |
@gerlowskija and @epugh the build errors you have probably occur because of the jvmargs in org.gradle.jvmargs=-Xmx2g -XX:TieredStopAtLevel=1 -Dfile.encoding=UTF-8 -Dkotlin.daemon.jvm.options\="-Xmx2048M" -XX:+UseParallelGC -XX:ActiveProcessorCount=1 \
--add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
--add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
--add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
--add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
--add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED Additionally, I added Starting with a clean build may also fix some issues, if it still doesn't build. |
Changing to |
I got it to run! So, thinks I would like to see are:
Very encouraging! |
I am eager to imlpement it since the first day you requested this, and will may start working on it soon. But I will probably not add it to the current POC because it will be too much to review.
A Kotlin client would be great to use here (it could eventually replace the current SolrJ implementation too). I have discussed some thoughts with @gerlowskija, but nothing noteworthy yet. I think if this POC gets accepted, we would have more reasons for introducing a additional admin client for Kotlin and support a bunch of new targets.
I think you can already package / build an installer for your platform by using one of the gradle tasks provided in the group
This is something that still raises concerns. Replacing the current webapp is one of the main goals of the new UI, and if it is shipped separately / outside of Solr project, we may not be able to deprecate and replace the current webapp with it and we will end up with "yet another Solr Admin tool". I think that if we notice further down the path that it gets out of hand, we may reconsider it and eventually remove the new module / UI completely (however, this may be a topic for the mailing list). I am not sure if that is what you meant, or if you simply want to see the UI as a plugin that can be loaded like any other plugin we have (sql, ltr etc). |
UI should absolutely be a standard component as always, imo not suited as a pkg. if we decide that solr should ship without a UI then that should be a separate decision, decoupled from this technology refresh. And if/when solr ships without a UI, then I believe the admin could be a separate app not running in the Solr JVM at all. |
Curious where we stand on this POC... If I remember right, one of the main questions that needs answered at this point is: "Do we think UI code in Kotlin is a better fit for our community of backend developers than JS or another 'traditional' frontend language?" I think I'm personally reassured on that front - even with this PR being massive I found it easier to understand than most JS I've spent time with. But it's a pretty big decision to make without more voices chiming in. @epugh @janhoy or others - do you guys have any thoughts/opinions on the language choice aspect of this? |
I plan to prepare a presentation + meeting where I go through the code, explain a few things and eventually do some live coding to show how to read the code, structure things and get started with a new component, so that people get more comfortable and give it a try themselves. I am just a bit short in time these weeks to prepare and announce something. Hopefully this helps in the decision. |
I've poked around a bit and am very interested in seeing it move forward! |
This commit creates a new module in Solr that sets up a frontend written with Compose and targeting browser (WASM) and desktop (JVM). The webapp is modified so that it opens the WASM Compose app when accessing /solr/compose. IMPORTANT: The jetty configuration is updated to include script-src: 'wasm-unsafe-eval' to allow WASM code execution which may be considered a security issue.
This commit adds a development flag to our gradle.properties that allows the selection of the build variant for the new AdminUI. When development enabled (default), Gradle will build a development instance and will have less secure configuration for the AdminUI to be able to attach debugging tools. When disabled, Gradle will optimize build output for the new Admin UI, but will also take longer to complete. Default is set to true to always build development locally and in CI/CD to avoid longer building times. Additionally, user is able to disable the new AdminUI via SOLR_ADMIN_UI_EXPERIMENTAL_DISABLED or by disabling the AdminUI. IMPORTANT: From this commit on, during releases, the development flag needs to be set explicitly to false, otherwise it will not generate an optimized Admin UI with improved CSP directives.
# Conflicts: # gradle/libs.versions.toml # gradle/template.gradle.properties # gradle/validation/dependencies.gradle # versions.lock
52fd7c4
to
c8f1f5b
Compare
https://issues.apache.org/jira/browse/SOLR-14414
Latest discussion thread: https://lists.apache.org/thread/knd5xgkswp83x92c0sclh7ghcob7bpfx
Proposal: SIP-7
Description
Due to the end-of-life of AngularJS (2022) we have to introduce a new Admin UI implementation that fully replaces the current webapp. SIP-7 as well as SOLR-14414 address this matter.
Solution
The proposed solution in this PR is an implementation of a new UI based on Kotlin and Compose Multiplatform that allows an integration of the new UI in the current webapp (until sufficient features are available to remove the old UI) as a WebAssembly app. The application can also be executed as a standalone desktop client (JVM-based) via
gradlew :solr:compose-ui:run
.Noticable (new) features:
For a better understanding of the files and classes see added dev-docs.
IMPORTANT Our current implementation for the checksum generation and license check does not support Kotlin multiplatform modules. This is a limitation that should probably be addressed in a separate PR.
The documentation assumes that we are able and allowed to publish dev builds of the desktop client via GitHub. Release flow for desktop client is not established yet, so it is subject to change.
Tests
Two example tests are provided for the existing components.
Checklist
main
branch../gradlew check
.