Skip to content

Auto-Quote database identifiers to avoid use of SQL keywords #237

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

Closed
gergelymolnarpro opened this issue Dec 5, 2019 · 10 comments
Closed

Comments

@gergelymolnarpro
Copy link

gergelymolnarpro commented Dec 5, 2019

Standardise hibernate.globally_quoted_identifiers, because when we create Java classes, we should not care about reserved SQL keywords.

Example: we can't create Release named class (or @entity(name="Release") ) if we use MySQL database with JPA api.

@andyjefferson
Copy link

+1 for auto-quoting database identifiers when necessary to avoid use of SQL keywords (not adding some Hibernate property), but then other persistence providers (e.g DataNucleus) have done that for years (before the JPA spec).

Suggest that you change the title of this issue to something like "Auto-Quote database identifiers to avoid use of SQL keywords" ... the current title is way too generic and could mean many things to many people

@gergelymolnarpro gergelymolnarpro changed the title Keep JPA higher level than SQL database Auto-Quote database identifiers to avoid use of SQL keywords Dec 18, 2019
@gavinking
Copy link
Contributor

The Hibernate team doesn't recommend the use of hibernate.globally_quoted_identifiers so I agree that JPA should not add anything like that.

@beikov
Copy link

beikov commented May 5, 2021

I guess what you are after is hibernate.auto_quote_keyword, but IMO this should be enabled by default in Hibernate. Not sure if the JPA spec should specify this behavior, but it's for sure a problem of Hibernate because it didn't enable this setting by default.

@gavinking
Copy link
Contributor

gavinking commented May 5, 2021

It's a hard call, and I'm not sure what the spec should say.

It's pretty hard for the spec to demand that the implementations auto-quote every single keyword on every single database they support. (How would you even test for compliance with that demand?)

On the other hand, I agree that it's a desirable default behavior.

I wondered if there was some sort of reasonable compromise: that the implementations should quote all ANSI SQL keywords, or something like that, but that doesn't seem to work either, since ANSI defines many irrelevant keywords.

I'm not sure. Tricky one.

@gavinking
Copy link
Contributor

gavinking commented May 5, 2021

Alright, so the thing I forgot about is case-sensitivity. Quoting identifiers makes them case-sensitive, which can cause problems since, when the column name is inferred from a Java field name, JPA just doesn't know the case on the database.

Now, it might seem like, well, we should quote it anyway, since using a keyword as a table or column name is sure to fail anyway, so we can just at least do a best-effort.

But that's not actually true. Lots of databases will accept a keyword column or table name depending on how it occurs syntactically. E.g. you can easily have a column called key on (some? most?) databases. And you don't always need to quote it on (some? most?) databases.

So, for example, if I have a table with a column named KEY in uppercase, and a Java field String key, then that works today. But if we turn on auto-quoting, that working code actually will break!

So it turns out that this issue is a whole lot more subtle than it looks.

On balance it might be best if the spec stays silent on this issue, and recommends, where necessary, the use of, e.g. @Column(name="\"KEY\"").

But I'm still not sure; it's a tough one.

@lukasj
Copy link
Contributor

lukasj commented May 5, 2021

The only thing I can imagine we could do here at the spec level would be to define ~2 properties - one for auto-quoting and the other for auto-uppercase naming - to leverage functionality providers already have support for, with semantics similar to query hints (either support or ignore). In general, I can agree with this being useful, OTOH the decision to turn this on should be always left up to the user, since consequences of having this on are really dependent not only on particular DB but also on its particular version (ie RANK in mysql 5 vs 8)

@gavinking
Copy link
Contributor

@lukasj Yeah, I think what you just said is about right.

OTOH, I'm skeptical that adding such optional settings to the spec is of much value, especially if we can't reasonably guarantee that the behavior is going to be very portable. (And I don't think we can really.)

@lukasj
Copy link
Contributor

lukasj commented May 5, 2021

@gavinking so just close this leaving it up to vendors to deal with this sort of issues? I'm fine with that, even from eclipselink's point of view.

@gavinking
Copy link
Contributor

Yes, on balance I think it's probably best.

@gavinking
Copy link
Contributor

@lukasj I think we can just close this one.

@gavinking gavinking closed this as not planned Won't fix, can't repro, duplicate, stale Aug 10, 2023
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

No branches or pull requests

5 participants