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

DevUI pages for Hibernate ORM #16764

Merged
merged 8 commits into from
Aug 10, 2021
Merged

Conversation

TomasHofman
Copy link
Contributor

@TomasHofman TomasHofman commented Apr 23, 2021

  • provide information about persistence units and their managed entities
  • allow user to generate create-schema script

Fixes #16553

@quarkus-bot quarkus-bot bot added area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE labels Apr 23, 2021
@TomasHofman
Copy link
Contributor Author

@Sanne I linked a PR with the first draft of the DevUI pages.

Vision: I think the useful things to show in the DevUI could be:

  • a list of existing persistence units,
  • a detail page for each PU showing a list of entities managed by given PU and a create-schema script.

Problems:

In order to generate the schema via the SchemaExport tool, I need to obtain the Metadata and ServiceRegistry instances of the persistence provider. I spend some time to figure out how to get them. I think my approach might not be entirely wrong but there are some further issues that need to be worked out.

  • The implementation creates a new instance of FastBootHibernatePersistenceProvider to generate the schema and adds some non API methods to this class.
  • The PreconfiguredServiceRegistryBuilder#buildNewServiceRegistry() method fails when called repeatedly (to generate new PersistenceProvider instance). I worked around it and added a comment to the place, but not sure if this is OK.
  • The PersistenceUnitsHolder#popRecordedState() throws away the metadata after the PU are created, which makes it impossible to generate the PersistenceProvider repeatedly. Can the metadata be kept in dev-mode?

@Sanne
Copy link
Member

Sanne commented Apr 26, 2021

@TomasHofman ! sorry for the delay, I've been off - need to catch up on things and then I'll have a better look at this. Thanks!

@TomasHofman
Copy link
Contributor Author

@Sanne thanks and no problem, I'm catching up on other stuff too...

@TomasHofman TomasHofman force-pushed the devui-db-schema-1.13 branch from 906907a to ed51b55 Compare April 29, 2021 08:50
@TomasHofman
Copy link
Contributor Author

I did some more work on the HTML, attaching screenshots:

embedded
persistence-units-ddls
entities
named-queries

@TomasHofman TomasHofman force-pushed the devui-db-schema-1.13 branch from ed51b55 to 675f909 Compare April 29, 2021 12:37
@geoand
Copy link
Contributor

geoand commented Apr 29, 2021

I can't comment on any of this stuff, but having a DevUI entry for Hibernate ORM with such info is an awesome idea!

Sanne
Sanne previously requested changes Apr 29, 2021
Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's starting to look cool :) But the ORM initialization code is a bit complex, I'll try to find some time to help with that.

I'm thinking there's a lot of functionality which we could add, but it's probably best to split it in smaller tasks.
For example, we could have:

  • also the "drop" script
  • a button to reset the DB state (actually runs the drop script, followed by the create and optionally the import.sql if there's any)

But IMO such additional things could be done via follow-up PRs, not least to get you some help from other people as well.
Let's focus first on the ORM boostrap changes and get the basics integrated first.

The "Names Query" table probably needs a column to show to which persistence unit it belongs; or alternatively maybe it's best to show multiple tables, one each for each PU.

@TomasHofman
Copy link
Contributor Author

I can squash the commits when we are done, but I will keep adding updates in separate commits for now to make it easier to review.

@TomasHofman
Copy link
Contributor Author

The "Names Query" table probably needs a column to show to which persistence unit it belongs; or alternatively maybe it's best to
show multiple tables, one each for each PU.

Good idea, I would use the separate-table-for-each-PU approach for "Managed entities" page too, so that the pages are consistent.

@yrodiere yrodiere force-pushed the devui-db-schema-1.13 branch from cd26682 to 7e87bb0 Compare August 2, 2021 10:57
@yrodiere yrodiere changed the base branch from 1.13 to main August 2, 2021 10:58
@yrodiere
Copy link
Member

yrodiere commented Aug 2, 2021

@TomasHofman @Sanne I pushed a commit to change how we retrieve the information: instead of being pulled at runtime, it's now generated on bootstrap. This works around all the problems related to generation of the schema after bootstrap.

Theoretically it should only have an impact in dev mode (not in tests nor in production), but let's wait for the build results :)

I also took the liberty of rebase this PR on the main branch since I don't think it makes sense to work on 1.13 right now, but let me know if that was a mistake.

@TomasHofman I know it's been a while, but do you think you could address the remaining comments? Feel free to ping me if you need help.

I believe y'all wanted to move to "one page per persistence unit", but after that's done I think it's going to be a great start and we can merge it?

@TomasHofman
Copy link
Contributor Author

TomasHofman commented Aug 2, 2021

@yrodiere that sounds great. I will take a look hopefully next week when I'm back from PTO and try to move it forward. Thanks!

Definitely the master branch - I had some problems with it at the time, which is why I started to work on 1.13.

@Sanne
Copy link
Member

Sanne commented Aug 2, 2021

Sounds great, many thanks @TomasHofman

@TomasHofman TomasHofman force-pushed the devui-db-schema-1.13 branch from 7e87bb0 to 270a50c Compare August 9, 2021 07:19
@TomasHofman
Copy link
Contributor Author

Hello, I updated the PR with additional changes.

  • Data for each PU are shown in separate tables now.
  • Showing both create and drop SQL scripts for each PU.
  • Some formatting changes.

I think this should cover all the requested changes, apart from providing a button to reset the database, which I would leave to a follow up PR.

Attaching current screenshots:

overview
pus
entities
named-queries

@TomasHofman TomasHofman changed the title WIP - draft of DevUI pages for Hibernate ORM DevUI pages for Hibernate ORM Aug 9, 2021
@yrodiere yrodiere force-pushed the devui-db-schema-1.13 branch from 270a50c to 5bec537 Compare August 9, 2021 13:43
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I pushed two more commits to address minor problems (one about dependencies, one about the javascript) and rebased it all on main.

Good to merge as soon as CI finishes the build.

@yrodiere yrodiere dismissed Sanne’s stale review August 9, 2021 13:45

Addressed concerns.

@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 9, 2021
@Sanne
Copy link
Member

Sanne commented Aug 9, 2021

Awesome, many thanks @TomasHofman and @yrodiere ! Lots of people will find this useful.

@gsmet
Copy link
Member

gsmet commented Aug 9, 2021

Would it be a problem to add a small test as done here: https://github.com/quarkusio/quarkus/pull/18738/files ?

The idea is mostly to check we don't break the pages with a 500 at some point.

@TomasHofman
Copy link
Contributor Author

I will have a look at the test.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 9, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 5bec537

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Test failures Logs Raw logs
✔️ JVM Tests - JDK 16

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/hibernate-orm/deployment

io.quarkus.hibernate.orm.sql_load_script.IntroducingDefaultImportScriptTestCase.testAddNewImportSql - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.netty.deployment.NettyProcessor#build threw an exception: java.lang.OutOfMemoryError: Metaspace
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
	at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:445)
	at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:405)
	at io.netty.buffer.UnpooledUnsafeDirectByteBuf._setByte(UnpooledUnsafeDirectByteBuf.java:183)
	at io.netty.buffer.AbstractByteBuf.writeByte(AbstractByteBuf.java:985)
	at io.netty.handler.codec.http.HttpObjectEncoder.<clinit>(HttpObjectEncoder.java:53)
	at java.base/java.lang.Class.forName0(Nati...

The import file name was not correctly handed over to the SchemaExport
utility.
@TomasHofman
Copy link
Contributor Author

I added test test and a fix for a problem where the import.sql file name was not determined correctly. Waiting for CI to complete.

@gsmet
Copy link
Member

gsmet commented Aug 10, 2021

@TomasHofman small detail: could we change the ordering with a comparator to get the <default> one first and then the named ones in alphabetical order?

@gsmet
Copy link
Member

gsmet commented Aug 10, 2021

Also I wonder if I wouldn't make the scripts in the persistence units view foldable (and folded by default). It sorta works for a small db but will be more challenging with a bunch of entities. I think we have an example in the score page of the RESTEasy Reactive Dev UI.

I would add a simple icon to fold/unfold. It would work well with your existing UI, you could put the copy button and the folding button at the end of the Create/Drop Script line, that way, we could copy even if folded.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 10, 2021

Failing Jobs - Building 66e1b37

Status Name Step Test failures Logs Raw logs
Native Tests - Misc4 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Native Tests - Misc4 #

📦 integration-tests/gradle

io.quarkus.gradle.nativeimage.CustomNativeTestSourceSetIT.runNativeTests line 18 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: "SUCCESS"
 but was: null
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at io.quarkus.gradle.nativeimage.CustomNativeTestSourceSetIT.runNativeTests(CustomNativeTestSourceSetIT.java:18)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688)
	at org.junit.jupiter.engine....

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this, it's a very good step forward. We can tweak things in follow-up PRs. Thanks!

@gsmet gsmet merged commit 19f7838 into quarkusio:main Aug 10, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 10, 2021
@quarkus-bot quarkus-bot bot added this to the 2.2 - main milestone Aug 10, 2021
@TomasHofman
Copy link
Contributor Author

Also I wonder if I wouldn't make the scripts in the persistence units view foldable (and folded by default). It sorta works for a small db but will be more challenging with a bunch of entities. I think we have an example in the score page of the RESTEasy Reactive Dev UI.

I would add a simple icon to fold/unfold. It would work well with your existing UI, you could put the copy button and the folding button at the end of the Create/Drop Script line, that way, we could copy even if folded.

Sounds like a good idea. I will create a new PR for that in couple of days after finish other work.

Thanks!

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

Successfully merging this pull request may close these issues.

Explore DDL scripts from Hibernate ORM in the DevUI
5 participants