Opened 9 years ago

Closed 4 years ago

#7140 closed Bug (wontfix)

Errors in escaping fieldnames in Oracle

Reported by: herwin@… Owned by: Matt Boersma
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: qsrf-cleanup oracle, quote, escape
Cc: herwin@…, Ian Kelly Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

According to the comments it's better to have your fieldnames in oracle quoted, but in our case this yielded to errors because the databasename had to be included in the tablename, which means all parts (database, table, field) have to be quoted seperately. The following snippet was the solution in our case (but I'm not sure if this always works, I'm no oracle expert)

--- db/backends/oracle/base.py     (revision 7510)
+++ db/backends/oracle/base.py     (working copy)
@@ -109,6 +109,7 @@
         # We simplify things by making Oracle identifiers always uppercase.
         if not name.startswith('"') and not name.endswith('"'):
             name = '"%s"' % util.truncate_name(name.upper(), self.max_name_length())
+            name = name.replace('.', '"."')
         return name.upper()

     def random_function_sql(self):

Change History (16)

comment:1 Changed 9 years ago by Ian Kelly

Cc: Ian Kelly added
Triage Stage: UnreviewedAccepted

The reason this wasn't done before was that it caused havoc with join aliases, which were generated by munging the table name. Now that queryset-refactor is merged into trunk, that's no longer an issue, so this may be a viable temporary fix until somebody has the time to work on implementing general schema support in #6148.

This solution doesn't work for doing cross-schema introspection, however. There's also a very minor backward incompatibility if anybody is actually using periods in their table names.

comment:2 Changed 8 years ago by George Vilches

Keywords: qsrf-cleanup added

comment:3 Changed 8 years ago by Jacob

milestone: 1.0

comment:4 Changed 8 years ago by Malcolm Tredinnick

Ian, this is much more your department than mine. There's nothing in the query construction code that is going to care about this. It treats the output of quote_name() as an opaque string and never tries to introspect it or change it in any way.

Whatever makes you happy is the Right Thing To Do. :-)

comment:5 Changed 8 years ago by cmarshal

Owner: changed from nobody to cmarshal
Status: newassigned

comment:6 Changed 8 years ago by cmarshal

Owner: changed from cmarshal to nobody
Status: assignednew

comment:7 Changed 8 years ago by Matt Boersma

Needs tests: set
Owner: changed from nobody to Matt Boersma
Status: newassigned

comment:8 Changed 8 years ago by Malcolm Tredinnick

@mboersma: can we get a quick thumbs up / thumbs down on this for 1.0? Is it really a bug, or just a nice-to-have feature to work around a lack of schema support. How practical is it not to include this? Or, if you're really confident that it's a solid fix, can it be checked in so that we know the resolution? The days are running out for people to have time to test things.

comment:9 Changed 8 years ago by Matt Boersma

I've been reviewing this, and I can say at least the patch does no harm--no tests break. But I've been trying to create a test case to see that it actually fixes the stated problem: that specifying db_table = '"schema.tablename"' works ok. My approach was to create a queryset, call qs.query.as_sql(), then change the Model._meta.db_table, create a new queryset and call qs.query.as_sql again. (It's a finicky test, but I special-cased it only to run on Oracle in the queries test suite.)

Unfortunately, in the second test I get something like:
SELECT "schema.tablename"."colname" FROM ....
which Oracle won't like.

So I'm not currently able to verify it fixes the problem. And given that the proper solution awaits in #6148 as Ian Kelly mentions, I'll have to say thumbs down for now.

(But I have plenty of time this weekend to attend to Django, so if someone has a strong argument for thumbs up, I will be glad to dig into this tomorrow.)

comment:10 Changed 8 years ago by Malcolm Tredinnick

milestone: 1.0post-1.0

In light of Matt's comment, let's move the milestone to get it off the showstopper list and it can be moved back if good arguments / working code appear.

comment:11 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:12 Changed 8 years ago by aprilmay

The proposed patch works fine here (with a legacy database, using db_table). Since it does not break anything and #6148 is still pending, is there any chance to have this patch for 1.1?

comment:13 Changed 6 years ago by Luke Plant

Severity: Normal
Type: Bug

comment:14 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:15 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:16 Changed 4 years ago by Anssi Kääriäinen

Resolution: wontfix
Status: assignedclosed

Im going to close this one. One can already use db_table = '"SOME_SCHEMA.SOME_TABLE"' if they want to. This ticket asks to allow usage of 'SOME_SCHEMA.SOME_TABLE'. No point in my opinion.

Note: See TracTickets for help on using tickets.
Back to Top