SOLR-14414: Introduce new UI (SIP-7) #2605
Conversation
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.
There was a problem hiding this comment.
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.
| @@ -20,6 +20,10 @@ import java.time.format.DateTimeFormatter | |||
|
|
|||
| plugins { | |||
| id 'base' | |||
| alias(libs.plugins.kotlinMultiplatform) apply false | |||
There was a problem hiding this comment.
[Q] Do you remember the rationale behind these build changes? Just trying to follow how everything works here...
There was a problem hiding this comment.
I added the plugins in the project gradle so that they are loaded once (currently used by two modules with different versions).
These changes are not necessary as such and came from a draft I made before but forgot to clean up. The draft was implementing a Solr client for Kotlin for generating classes from OAS, and I had to sync the OpenAPI plugin versions.
If I remember correct, the plugin for Kotlin multiplatform and Compose should be loaded at project level (haven't tried it at module level) and only applied to modules were necessary.
The multiplatform plugin is used for supporting individual targets in the gradle module level (hence the commonMain, jvmMain, wasmJsMain sub-modules / directories in the new ui module). This way, we can provide platform specific code if necessary (see expect-actual declarations).
| Compose comes with many accessibility features that can be used to improve the user experience. | ||
|
|
||
| The simplest form of accessibility in UI is probably the responsiveness of the UI. this is | ||
| realized with `WindowSizeClass`. Some compsables may use a wrapper (usually suffixed with `Content`) |
There was a problem hiding this comment.
[0] wanted to flag the typo in "compsables" in case this PR comes out of draft mode at some point.
| @@ -0,0 +1,83 @@ | |||
| = Technology Overview | |||
There was a problem hiding this comment.
[+1] These are great docs, and I love that they really anticipate the complete lack of knowledge the rest of us have when it comes to Kotlin. Hopefully they'll be helpful to folks!
The sheer number of libraries and technologies here is a bit intimidating. 😬 But I'm hoping this is just a side effect of seeing all the relevant libraries listed out. I'm sure they're easy to pick up one-by-one in the code with these docs as a handy reference.
There was a problem hiding this comment.
I think "sheer number" is okay, have you ever looked at a React app? "Now downloading library 234 of 789"....
There was a problem hiding this comment.
I listed all libraries here in detail, even though some of them are common libraries in Kotlin / Compose projects. I did this only because most of the libraries are for non-kotlin folks new, so having some reference should be helpful for getting started. Maybe in the long run it doesn't have to be that detailed.
There was a problem hiding this comment.
Yeah, it ended up not being too bad once I got into the actual PR code. The docs set great context though 👍
| = Testing and Deployment | ||
|
|
||
| == Testing | ||
|
|
There was a problem hiding this comment.
[0] Not important, just leaving this comment as a reminder that these docs should be populated at some point if this moves beyond POC.
| @@ -0,0 +1,65 @@ | |||
| [versions] | |||
There was a problem hiding this comment.
[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.
[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.
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.
| autoMirror = false, | ||
| ).apply { | ||
| materialPath { | ||
| moveTo(25.7013f, 44.178f) |
There was a problem hiding this comment.
[Q] Wait, is this code drawing a Solr logo from individual points? 🤯 How did you figure all these float coordinate values out, and how long did that take? Man, the work you put into this PR just seems astronomical.
The project has png and other logo artifacts that we could use, unless there's some benefit to drawing the logo "by hand" as this code does?
There was a problem hiding this comment.
Since we are working in a "canvas" more or less, drawing with points is quite common for icons and logos. SVGs have the benefit that they can be styled, colored and scaled too, which is the reason I chose to use a "native" drawing approach. I only had to translate the SVG data into Compose, which I did manually here because of the coloring / theming, but there are also libraries that can do that for SVG icons.
In UI development, PNGs and JPGs are normally provided in multiple scales to cover a wide range of screen resolutions to show the image always as sharp as necessary. SVGs on the other hand can simply be scaled and are therefore prefered (if available).
There was a problem hiding this comment.
Oh ok, wow. I never would've guessed that.
Or, I guess I knew about wanting the logo to scale crisply, but I figured that'd involve using an svg image file as an asset, rather than writing code to draw points directly.
But makes sense!
There was a problem hiding this comment.
I really don't want to discourage this effort in anyway, but I'm unclear on the benefit of a programmatically drawn SVG vs an xml formatted SVG file that could be viewed edited with image editing programs, or even an ide. I'm 100% for an SVG version that stays crisp of course! Just wondering if this is the most maintainable format.
Intellij for example will happily display xml formated svg while you edit directly:
And there may be folks with skill using other more advanced image editing tools.
There was a problem hiding this comment.
I agree and this is not common practice in general (should be avoided if possible for the reasons you mention).
In compose, users normally import SVGs to convert them to usable resource files (a different XML that is common in Android), which still can be rendered in IntelliJ and Android Studio and is the common practice and format for resource files that are image vectors. Compose MP supports SVGs too and therefore it should be prefered.
However, this format does only support a single tint or predefined colors. In the current case, the logo is themed and changes colors based on the theme. This is only achievable (afaik) if we have direct access to the groups / content of the drawing, which is not the case when we use XML and the Compose resources.
And just for reference, Material Icons in Compose are also written and distributed in a similar format, but the icons are coming from a library and therefore not a developer's problem.
In general, if icons are not available through a library like the material icons or material icons extended, we would use the import option to import an SVG and convert it to a Compose XML, then load it as a resource. This logo is only an exception. Providing pre-colored XMLs of the logo for each theme would be the alternative approach for loading the logo in the correct colors.
I would provide a dev-guide in the future if this POC gets accepted for how to import SVGs and from where to get icons to not have to write such code like here.
P.S. we still can use Compose and IntelliJ to render a preview of the logo regardless, so that we have a preview like with the XMLs.
There was a problem hiding this comment.
also, honestly this is a bit of bike shedding... ;-). If someone really wants .svg, they can submit a PR to convert it ;-). There are lots of places in our code base that maybe are a bit idiosyncratic based on the person who wrote the code, and if we don't have an explicit community standard, then I think thats okay ;-)
| component = child.component, | ||
| modifier = Modifier.fillMaxWidth() | ||
| .verticalScroll(scrollState) | ||
| .padding(16.dp), |
There was a problem hiding this comment.
[Q] UI Novice Question:
How do you arrive at padding, width, etc. values like the one here? Is it generally guess-and-check using your best judgement on a few form-factors? Or is there a more guided/structured way to do it?
There was a problem hiding this comment.
The Figma designs I created in advance follow the 8 point grid system (multiples of 8) that is a common practice for choosing spacings and dimensions. There are many blog posts that explain various benefits of this system, The power of the 8pt grid system in design and everything you should know about 8 point grid system in UI design are just two of them. I mostly follow the guidelines from Material 3 too. Information about spacing and other stuff can be found in the guidelines.
There was a problem hiding this comment.
Perfect, I've got some reading to do, thanks!
|
|
||
| package org.apache.solr.composeui.ui.navigation | ||
|
|
||
| /** |
There was a problem hiding this comment.
[+1] Really appreciate these comments, thank you!
| * critical errors or warnings. | ||
| */ | ||
|
|
||
| val primaryLight = Color(0xFF9C1F00) |
There was a problem hiding this comment.
[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.
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.
| * Function that returns a simple HTTP client that is preconfigured with a base | ||
| * URL. | ||
| */ | ||
| fun getDefaultClient() = HttpClient { |
There was a problem hiding this comment.
[Q] I guess this is where authentication logic would need to be hooked in at some point, to ensure that client requests have whatever headers, etc. are necessary for Solr to accept the requests?
There was a problem hiding this comment.
this is something that I personally is really important to see... How does authentciation work?
There was a problem hiding this comment.
Exactly. All classes that need a client define it in the parameters, so that it can easily be injected wherever necessary. Right now, the method is called at root level and passed down. The component responsible for authentication would use the "default client" for the authentication and pass down a differnt (or the same) client with the necessary headers once authenticated.
It is worth noting that I explicitly avoided to use an injection framework, because I do not like the "magic" that is happening behind the scenes. It often caused a lot of confusion, especially to new devs. This approach has also proven to be easier for testing.
There was a problem hiding this comment.
How does authentciation work?
I would probably implement authentication as another UI component that acts as a decorator for all the underlying components. It would be loaded directly under the root component in the UI layers (so to speak) and check if there are credentials (or any other authentication information) present, or if an authentication is necessary. In case auth data is present, it is used and the auth component proceeds in the "authenticated route". If there is no data present, it proceeds with the "unauthenticated route", that shows the user the login screen.
Additionally, all components with a state have an AppComponentContext that can be extended with authorization data and user identity. That data can be used to update the component's state accordingly and hide for example UI elements if the user is not authorized to use.
So Authentication and Authorization becomes a responsibility of "just another component" and may be part of the components' context.
The getDefaultClient() function is only used for convenience here. The component responsible for auth would probably provide the logic for creating an authenticated / authorized client and pass it down to all the other components that need an HttpClient. This way, the component can also react on auth expiration, token refresh etc, if not handled by the HttpClient itself, and switch the routes on demand.
And last but not least, the Auth component(s) and related store(s) may be platform-specific or platform-aware, since depending on the platform, different secure stores may be used for storing credentials / auth data, as well as different auth mechanisms may be supported depending on the platform the application is running (like Kerberos auth).
|
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. |
| @@ -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.
wasm! Fun! I heart that tech. ;-)
There was a problem hiding this comment.
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-UNNAMEDAdditionally, I added Starting with a clean build may also fix some issues, if it still doesn't build. |
| //apply from: file('gradle/hacks/turbocharge-jvm-opts.gradle') | ||
| //apply from: file('gradle/hacks/dummy-outputs.gradle') |
There was a problem hiding this comment.
Note that I had to disable some of the hacks because of some issues I was facing with them. Maybe this causes some gradle processes from the automation scripts to fail now?
|
Changing to |
|
|
||
|
|
||
| ----------------------------------------------------------- | ||
| SIL OPEN FONT LICENSE Version 1.1 - 26 February 2007 |
There was a problem hiding this comment.
Any issues with this license in an ASF project?
| ~ limitations under the License. | ||
| --> | ||
|
|
||
| <resources> |
There was a problem hiding this comment.
is this a default pattern? If we are never going to translate solr into various languages, do we even need this? Just thinking about how to reduce the layers of indirection....
There was a problem hiding this comment.
Even if we would support english only, it is not a bad idea to write out the strings in a separate file and reference those. This keeps the code clean from long texts and allows localization in the future, if we decide to provide translations at some point.
|
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! |
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.Noticable (new) features:
For a better understanding of the files and classes see added dev-docs.
Tests
Tests will be provided soon.
Checklist
mainbranch../gradlew check.