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

Add support for SQL script file being picked up at Hibernate ORM start time #511

Closed
FroMage opened this issue Jan 15, 2019 · 32 comments
Closed
Assignees
Milestone

Comments

@FroMage
Copy link
Member

FroMage commented Jan 15, 2019

I'm missing at least the ability to set javax.persistence.sql-load-script-source to remove my persistence.xml and set it in the config file. Though frankly I suspect that it could default to META-INF/load.sql if present, no?

Also, is there any reason why the property shamrock.hibernate.schema-generation.database.action is named this long? Can't shamrock.hibernate.schema-generation do the job?

WDYT?

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

Right we plan to default to a unique name for the import script.

Although I'd not mind using the existing de facto standard in Hibernate: /import.sql ? Might even work already, didn't test it though.

I suspect the reason to call it schema-generation.database.action is that it reflects the JPA standard name. (Which is literally that same string).

@FroMage
Copy link
Member Author

FroMage commented Jan 15, 2019

I'm fine with import.sql too. But should it be in META-INF?

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

I wouldn't put it into META-INF, as it has never been there in the past. So, unless you have strong reasons to move it?

Either way, I quickly tested an /import.sql and the resource was ignored. We'll fix that..

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

Proposal

  • add support for choice & naming preferences by implementing sql-load-script-source.
  • keep the /import.sql as default to ease ports.

@FroMage
Copy link
Member Author

FroMage commented Jan 15, 2019

It's just where it was in rest-http-crud, I don't mind where it is.

I suspect the reason to call it schema-generation.database.action is that it reflects the JPA standard name. (Which is literally that same string).

I think for frequent settings we should shorten it. Now, if we want to support any setting, we could say that any setting starting with shamrock.hibernate.property.* is copied as-is with that prefix removed, no? This way we support every property even if we don't have shortcuts for them yet.

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

we could say that any setting starting with [..]

We floated such idea at the meeting, remember? The big problem is it kills any possibility to spot typos, usage of abandoned properties, etc..

In my experience that's a big deal. Lots of trouble with Hibernate users over the years because we couldn't 100% validate the config props - for various similar reasons.

I'd really like to be able to validate all options are consumed as intended, be able to (at least) warn otherwise.

@FroMage
Copy link
Member Author

FroMage commented Jan 15, 2019

OK, fine :)

@emmanuelbernard
Copy link
Member

I don't see why you would need a way to custom import.sql (i.e. be able to set javax.persistence.sql-load-script-source). Do you have a valid reason or is that just another of the engineering option to the user relapse?
image

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

You'd probably want to use a different script for dev/prod/whatever, and having a single value to change (or set some property) is usually the least annoying way to achieve that.

Alternatives?

@emmanuelbernard
Copy link
Member

If that's the case then we can look for load.sql and load-profile.sql like load-dev.sql

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

I like that direction, in principle... but when they are the same you expect people to maintain copies?

@emmanuelbernard
Copy link
Member

the advantage of load over import is that it's more consistent if one day we also want the create and drop scripts to be picked up

@emmanuelbernard
Copy link
Member

no when they are the same, they just go for load.sql

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

@emmanuelbernard so which one is it going to be?

  1. load.sql because your opinion today is that it fits better
  2. import.sql because that's what everyone is used to

Incidentally, don't think of it as "loading data" eclusively. I used to have scripts and table admin steps in there too; it's more accurate to state that we just import the SQL which is in there - whatever it is.

@emmanuelbernard
Copy link
Member

In JPA for table admin stuff you have the create and drop options. Or are you talking about something else. Frankly, we should look at the JPA doc to see why they have then separated.

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

In JPA for table admin stuff you have the create and drop options.

There's far more in a database than what's defined by the JPA annotations.. I'd really need it to run additional SQL.

@emmanuelbernard
Copy link
Member

@Sanne you don't get me, JPA defines three lifecycles to alter the DB with SQL. create, drop and load. I forgot why and we need to look again.

@kenfinnigan
Copy link
Member

Isn't this really only a dev time type solution?

Do we really want production deployments with import.sql being used? What happens when you spin up more containers, gets called multiple times?

@emmanuelbernard
Copy link
Member

The multi container is a point. That would require a whole infrastructure. to propose that.
Note that cron jobs have the same problem don't they?

@emmanuelbernard
Copy link
Member

Thinking about this one vs multiple files, I think we could decide to go for a single file for now as it suits Hibernate ORM quite well. in which case we could reuse import.sql

@kenfinnigan
Copy link
Member

I believe they would yes

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

say I want to test the app on two different databases, I'll need to be able to switch between two different scripts.

@kenfinnigan
Copy link
Member

To me that reinforces this is purely a dev only concern.

Could it be something that's only active/available when shamrock:dev is used?

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

sure it absolutely is a dev concern.

Still, having the option to switch among files among various stages of dev is very useful; having to replace the file/resource with build tools is just not as user friendly as being able to pick one.

@emmanuelbernard
Copy link
Member

What are you arguing for then @Sanne ?

@emmanuelbernard
Copy link
Member

Note that multi database support is not the 90% use case.

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

What are you arguing for then?

That taking away the flexibility of not being able to choose the resource script to import is not user friendly at all.

Sure users will find their way around it, but no other solution is as simple as allowing to set the name.

Which is what I said at #511 (comment) .

I totally get it that we want to limit the options, but IMO it's good to e.g. not allow people to choose among a dozen different implementation pools, but removal of this one goess directly against day to day usability.

@emmanuelbernard
Copy link
Member

You can use persistence.xml we don't forbid that. Here we go for the mainstream usage with no persistence.xml (and can loosen up over time). Assuming a single database of a single type. Do you want the option to change the script depending on the environment (which one proposal offers), or is the dev mode a good indicator of using the one single script file?

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

hum ok fine by me. I thought we'd generally want to discourage persistence.xml even for non-toy applications.

N.B. I'm still pretty sure I'd need multiple files even when working on a single database type, this is just such an extremely common need.

BTW following this line of thought wouldn't you remove connection pool sizes as well? That's definitely just "final prod tuning"

@emmanuelbernard
Copy link
Member

Let's pick up the phone tomorrow because you see it as toy only limitation and I don't.

@Sanne
Copy link
Member

Sanne commented Jan 15, 2019

I really didn't see it as a toy. But when I challenged that this is very useful for day to day dev, your answer was "you can use the persistence.xml" which seems to imply I'll always need a persistence.xml for serious development.

@emmanuelbernard
Copy link
Member

#538

@emmanuelbernard emmanuelbernard changed the title Config persistence.xml missing properties Add support for SQL script file being picked up at Hibernate ORM start time Jan 16, 2019
@emmanuelbernard emmanuelbernard added this to the 0.7.0 milestone Jan 17, 2019
maxandersen added a commit to maxandersen/quarkus that referenced this issue Nov 5, 2022
…uarkusio#506) (quarkusio#511)

* docs: Clarify that .jsh are not cached and no native support (fixes quarkusio#506)

* Update readme.adoc

Co-authored-by: Max Rydahl Andersen <max.andersen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants