Code

Opened 6 years ago

Closed 2 years ago

#7140 closed Bug (wontfix)

Errors in escaping fieldnames in Oracle

Reported by: herwin@… Owned by: mboersma
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: qsrf-cleanup oracle, quote, escape
Cc: herwin@…, ikelly 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):

Attachments (0)

Change History (16)

comment:1 Changed 6 years ago by ikelly

  • Cc ikelly added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 6 years ago by gav

  • Keywords qsrf-cleanup added

comment:3 Changed 6 years ago by jacob

  • milestone set to 1.0

comment:4 Changed 6 years ago by mtredinnick

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 6 years ago by cmarshal

  • Owner changed from nobody to cmarshal
  • Status changed from new to assigned

comment:6 Changed 6 years ago by cmarshal

  • Owner changed from cmarshal to nobody
  • Status changed from assigned to new

comment:7 Changed 6 years ago by mboersma

  • Needs tests set
  • Owner changed from nobody to mboersma
  • Status changed from new to assigned

comment:8 Changed 6 years ago by mtredinnick

@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 6 years ago by mboersma

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 6 years ago by mtredinnick

  • milestone changed from 1.0 to post-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 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:12 Changed 5 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 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:14 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:15 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:16 Changed 2 years ago by akaariai

  • Resolution set to wontfix
  • Status changed from assigned to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.