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

Enable quoting of all identifiers by default #1

Closed
wants to merge 1 commit into from
Closed

Conversation

Sanne
Copy link
Owner

@Sanne Sanne commented Apr 8, 2019

@FroMage what wou you think of this approach? cc/ @gsmet

On the upside it will consistently create/update the table names...

but I don't think you'll be able to use the import.sql as easily anymore; maybe that's ok and we switch to the Flyway approach :)
Writing SQL directly in the DB also becomes a bit messy, you'll have to be identifiers consistently.

DO NOT MERGE

@FroMage
Copy link

FroMage commented Apr 9, 2019

It's a PR in your own repo, you can merge it if you want ;)

So what does that mean for import.sql then?

I won't be able to insert into foo … if I have @Entity class Foo {}?

@gsmet
Copy link

gsmet commented Apr 9, 2019

So my position on this one is that it should work with the default settings.

From what I see in the original issue, it seems to be really the default behavior not working. I'm pretty sure it used to work back in the time because I used update all the time with my H2/PostgreSQL databases in the past.

@Sanne
Copy link
Owner Author

Sanne commented Apr 9, 2019

It's a PR in your own repo, you can merge it if you want ;)

So what does that mean for import.sql then?

I won't be able to insert into foo … if I have @Entity class Foo {}?

That's right, you'd have to use insert into Foo, except that you'll have to quote "Foo" otherwise psql will downcase it first and then complain the table foo doesn't exist (as the physical name is Foo).

When trying an import.sql with this syntax it did reject the script though, haven't figured why yet but I guess such escaping is not supported.

@gsmet

So my position on this one is that it should work with the default settings.

Well we can agree on the ideal :) but we need to decide how to do that in practice, as each option I can think of will backfire on some other area.

My personal experience has always been fine with this as well, but I suspect in my case this is caused as I'd regularly use my own custom naming strategies as I always needed 100% control on the schema output.

@FroMage
Copy link

FroMage commented Apr 9, 2019

Yes, it not only used to work, but also drop-and-create manages to drop the right tables, while update does not update the right ones, so it's really a Hibernate bug IMO. But regardless I agree we should fix it here to have a quick fix.

I'm not sure the strategy to use quoting is the right way to fix this, though, because it makes even using psql harder, no?

Do note that at least another user reported this not working with H2 in quarkusio#1886 so it's not just postgres.

@FroMage
Copy link

FroMage commented Apr 9, 2019

What does @emmanuelbernard think?

@gsmet
Copy link

gsmet commented Apr 9, 2019

Agreed. I think we really need to understand the root of the issue.

@Sanne
Copy link
Owner Author

Sanne commented Apr 9, 2019

You can't have the old style back so let's look ahead. Rather than keep repeating "fix the bug" tell me how you think this is supposed to work ;)

@FroMage
Copy link

FroMage commented Apr 9, 2019

So it appears that H2 will upper-case everything that is not quoted. While PG will lowercase everything non-quoted and even quoted things if they don't contain any uppercase letters. I think requiring users to quote their table names when interacting with the DB is the nuclear option to catch a fly. Especially because Java guidelines mean that every table name will be CamelCased, so quoting will be required by any manual DB interaction.

I don't see the naming strategy listed as options in quarkus-hibernate-orm so I can't test them.

All else on the side, do you think fixing update would be hard or impossible? Without talking about delays.

@Sanne
Copy link
Owner Author

Sanne commented Apr 9, 2019

It's a non trivial problem as the databases just don't deal with naming consistently and there's not much we can do about that; but I think we can fix it or at least push some reasonable default naming strategy depending on the dialect.

But we need to agree on what strategy exactly...

FYI in the ORM core team the consensus has been that the status quo is as good as it can get: people really are expected to be precise with their naming and plugging a user custom strategy is very common, and is the least surprising option as they will have made an explicit choice.

But you're advocating an approach of "users shouldn't be bothered at all" .. wich has my sympathies but we've not been able to achieve that so far.

I think requiring users to quote their table names when interacting with the DB is the nuclear option to catch a fly.

Well it has some more benefits; just try having an entity called "User" and see... :-(

But sure it's nuclear as it makes all other tools more annoying to use; would be great if all tools used quoting by default.

So "quoting enabled by default" is totally unreasonable in ORM upstream as it would break existing applications, but it's something we could consider for Quarkus given the focus on greenfield apps.

@Sanne
Copy link
Owner Author

Sanne commented Apr 9, 2019

So this is how it would look like, if we were to apply the change in this PR:

Not too bad?

@gsmet
Copy link

gsmet commented Apr 9, 2019

What I fail to understand is that I used PostgreSQL with update for years without any issue.

Did you reproduce it with PostgreSQL? I see the OP uses H2. For H2, I think we dropped the schema so I don't have any data for it.

I don't have the time right now to check it out, please don't rush it.

@Sanne
Copy link
Owner Author

Sanne commented Apr 9, 2019

@gsmet sure in fact I'm working with PostgreSQL only.

Sanne pushed a commit that referenced this pull request May 8, 2019
fork refresh from quarkus master
Sanne pushed a commit that referenced this pull request Jun 3, 2019
@Sanne Sanne closed this Nov 14, 2019
@Sanne Sanne deleted the PGSQL_Casing branch November 14, 2019 20:33
Sanne pushed a commit that referenced this pull request Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants