Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#21821 closed Bug (fixed)

ORM lookup refactor broke Oracle tests

Reported by: Tim Graham Owned by: Anssi Kääriäinen
Component: Database layer (models, ORM) Version: dev
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Tests fail to run with the following traceback:

Traceback (most recent call last):
  File "./runtests.py", line 384, in <module>
    options.failfast, args)
  File "./runtests.py", line 226, in django_tests
    test_labels or get_installed(), extra_tests=extra_tests)
  File "/home/tim/code/django/django/test/runner.py", line 147, in run_tests
    old_config = self.setup_databases()
  File "/home/tim/code/django/django/test/runner.py", line 109, in setup_databases
    return setup_databases(self.verbosity, self.interactive, **kwargs)
  File "/home/tim/code/django/django/test/runner.py", line 299, in setup_databases
    verbosity, autoclobber=not interactive)
  File "/home/tim/code/django/django/db/backends/creation.py", line 367, in create_test_db
    test_database=True)
  File "/home/tim/code/django/django/core/management/__init__.py", line 167, in call_command
    return klass.execute(*args, **defaults)
  File "/home/tim/code/django/django/core/management/base.py", line 291, in execute
    output = self.handle(*args, **options)
  File "/home/tim/code/django/django/core/management/commands/migrate.py", line 149, in handle
    emit_post_migrate_signal(created_models, self.verbosity, self.interactive, connection.alias)
  File "/home/tim/code/django/django/core/management/sql.py", line 244, in emit_post_migrate_signal
    using=db)
  File "/home/tim/code/django/django/dispatch/dispatcher.py", line 198, in send
    response = receiver(signal=self, sender=sender, **named)
  File "/home/tim/code/django/django/contrib/auth/management/__init__.py", line 85, in create_permissions
    ctype = ContentType.objects.db_manager(using).get_for_model(klass)
  File "/home/tim/code/django/django/contrib/contenttypes/models.py", line 49, in get_for_model
    defaults={'name': smart_text(opts.verbose_name_raw)},
  File "/home/tim/code/django/django/db/models/manager.py", line 77, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/tim/code/django/django/db/models/query.py", line 416, in get_or_create
    return self.get(**lookup), False
  File "/home/tim/code/django/django/db/models/query.py", line 345, in get
    num = len(clone)
  File "/home/tim/code/django/django/db/models/query.py", line 121, in __len__
    self._fetch_all()
  File "/home/tim/code/django/django/db/models/query.py", line 962, in _fetch_all
    self._result_cache = list(self.iterator())
  File "/home/tim/code/django/django/db/models/query.py", line 264, in iterator
    for row in compiler.results_iter():
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 693, in results_iter
    for rows in self.execute_sql(MULTI):
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 766, in execute_sql
    sql, params = self.as_sql()
  File "/home/tim/code/django/django/db/backends/oracle/compiler.py", line 43, in as_sql
    with_col_aliases=True)
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 107, in as_sql
    where, w_params = self.compile(self.query.where)
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 78, in compile
    return node.as_sql(self, self.connection)
  File "/home/tim/code/django/django/db/models/sql/where.py", line 105, in as_sql
    sql, params = qn.compile(child)
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 78, in compile
    return node.as_sql(self, self.connection)
  File "/home/tim/code/django/django/db/models/lookups.py", line 120, in as_sql
    lhs_sql = connection.ops.field_cast_sql(db_type, field_internal_type) % lhs_sql
  File "/home/tim/code/django/django/db/backends/oracle/base.py", line 277, in field_cast_sql
    if db_type and db_type.endswith('LOB'):
AttributeError: 'CharField' object has no attribute 'endswith'

Change History (14)

comment:1 by Anssi Kääriäinen <akaariai@…>, 10 years ago

Resolution: fixed
Status: newclosed

In f468662e2495292356e5fd75241621563893fd4f:

Fixed #21821 -- db_type argument for field_cast_sql

The db_type argument for connection.ops.field_cast_sql wasn't correctly
set after merge of custom lookups patch.

comment:2 by Tim Graham, 10 years ago

Resolution: fixed
Status: closednew

Thanks, tests are now running. There are some errors of this type:

======================================================================
ERROR: test_unique_for_date (validation.test_unique.PerformUniqueChecksTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/validation/test_unique.py", line 120, in test_unique_for_date
    p.full_clean()
  File "/home/tim/code/django/django/db/models/base.py", line 998, in full_clean
    self.validate_unique(exclude=exclude)
  File "/home/tim/code/django/django/db/models/base.py", line 808, in validate_unique
    date_errors = self._perform_date_checks(date_checks)
  File "/home/tim/code/django/django/db/models/base.py", line 928, in _perform_date_checks
    qs = model_class._default_manager.filter(**lookup_kwargs)
  File "/home/tim/code/django/django/db/models/manager.py", line 77, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/tim/code/django/django/db/models/query.py", line 685, in filter
    return self._filter_or_exclude(False, *args, **kwargs)
  File "/home/tim/code/django/django/db/models/query.py", line 703, in _filter_or_exclude
    clone.query.add_q(Q(*args, **kwargs))
  File "/home/tim/code/django/django/db/models/sql/query.py", line 1287, in add_q
    clause, require_inner = self._add_q(where_part, self.used_aliases)
  File "/home/tim/code/django/django/db/models/sql/query.py", line 1316, in _add_q
    current_negated=current_negated, connector=connector)
  File "/home/tim/code/django/django/db/models/sql/query.py", line 1186, in build_filter
    condition = self.build_lookup(lookups, col, value)
  File "/home/tim/code/django/django/db/models/sql/query.py", line 1091, in build_lookup
    next = lhs.get_lookup(lookup)
  File "/home/tim/code/django/django/db/models/sql/datastructures.py", line 25, in get_lookup
    return self.output_type.get_lookup(name)
  File "/home/tim/code/django/django/db/models/lookups.py", line 12, in get_lookup
    return self.class_lookups[lookup_name]
TypeError: unhashable type: 'list'

Let me know if you need more details.

comment:3 by Shai Berger, 10 years ago

FWIW, I will look at this tomorrow if not solved by then.

comment:4 by Anssi Kääriäinen <akaariai@…>, 10 years ago

In 994c842bc9266f3d1e36b10e9365d74844ccb28c:

Fixed Oracle query failures caused by lookup refactor

Refs #21821.

comment:5 by Anssi Kääriäinen, 10 years ago

I think I got the latest failure fixed. I don't have Oracle available just now so I can't verify that Oracle passes all tests. Leaving as unfixed until all tests pass.

comment:6 by Anssi Kääriäinen, 10 years ago

While working on these issues I recalled that I forgot to add in lookup support for Oracle when there are more than 1000 parameters for the lookup. This commit should fix it: https://github.com/akaariai/django/commit/ac8e62768f13ae2ece4288539eb753b753ae47cf. The commit also cleans up non-necessary rhs parameter for process_rhs().

I can't test the patch on Oracle before tomorrow. This is complex enough patch to require testing on Oracle, so I'll wait until I can test the patch.

comment:7 by Tim Graham, 10 years ago

New error with the above patch applied to master:

======================================================================
ERROR: test_regex (lookup.tests.LookupTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/lookup/tests.py", line 507, in test_regex
    ['<Article: f>', '<Article: fo>', '<Article: foo>', '<Article: fooo>'])
  File "/home/tim/code/django/django/test/testcases.py", line 829, in assertQuerysetEqual
    items = six.moves.map(transform, qs)
  File "/home/tim/code/django/django/db/models/query.py", line 140, in __iter__
    self._fetch_all()
  File "/home/tim/code/django/django/db/models/query.py", line 962, in _fetch_all
    self._result_cache = list(self.iterator())
  File "/home/tim/code/django/django/db/models/query.py", line 264, in iterator
    for row in compiler.results_iter():
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 693, in results_iter
    for rows in self.execute_sql(MULTI):
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 766, in execute_sql
    sql, params = self.as_sql()
  File "/home/tim/code/django/django/db/backends/oracle/compiler.py", line 40, in as_sql
    with_col_aliases=with_col_aliases)
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 107, in as_sql
    where, w_params = self.compile(self.query.where)
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 78, in compile
    return node.as_sql(self, self.connection)
  File "/home/tim/code/django/django/db/models/sql/where.py", line 105, in as_sql
    sql, params = qn.compile(child)
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 78, in compile
    return node.as_sql(self, self.connection)
  File "/home/tim/code/django/django/db/models/lookups.py", line 131, in as_sql
    operator_plus_rhs = self.get_rhs_op(connection, rhs_sql)
  File "/home/tim/code/django/django/db/models/lookups.py", line 135, in get_rhs_op
    return connection.operators[self.lookup_name] % rhs
KeyError: 'regex'

comment:8 by Anssi Kääriäinen, 10 years ago

I have three different fixes for custom lookups:

First two are oracle-only, last might affect other backends, too.

These were found by checking what sql.where make_atom() does differently compared to class based lookups.

comment:9 by Anssi Kääriäinen <akaariai@…>, 10 years ago

In a9f171151814c03fcb0dcc87b153397e90562ea2:

Fixed Oracle in lookup when more than 1000 params

Refs #21821.

comment:10 by Anssi Kääriäinen <akaariai@…>, 10 years ago

In b86321e9a42652909d9c3373f3b97fcc9ed2c290:

Fixed regex lookup on Oracle

Refs #21821

comment:11 by Anssi Kääriäinen <akaariai@…>, 10 years ago

In 1c360dbbf56b76e1df7c945458ae2987306fcfcd:

Use date_extract_sql() for DateFields

Refs #21821

comment:12 by Anssi Kääriäinen <akaariai@…>, 10 years ago

In 980eda01909c9fd5c5e16f276ca663e71371a0a2:

Fixed custom_lookups tests for Oracle

Refs #21821

comment:13 by Anssi Kääriäinen, 10 years ago

Resolution: fixed
Status: newclosed

This one should be now fixed.

Interestingly tests seem to pass even without 1c360dbbf56b76e1df7c945458ae2987306fcfcd. I haven't ran full test suite on Oracle, but other databases pass without that, and timezones tests pass on Oracle also without that commit.

I noticed that the __search lookup isn't tested at all. It is only supported on MySQL. It likely doesn't work currently, but that is food for another ticket.

Edit note: One reason for the high amount of regressions is that I intentionally did not try to move existing make_atom() logic to class based lookups. The make_atom logic was somewhat weird in some places. The end result should be much cleaner now.

Last edited 10 years ago by Anssi Kääriäinen (previous) (diff)

in reply to:  13 comment:14 by Jakub Roztočil, 10 years ago

Replying to akaariai:

This one should be now fixed.

Interestingly tests seem to pass even without 1c360dbbf56b76e1df7c945458ae2987306fcfcd. I haven't ran full test suite on Oracle, but other databases pass without that, and timezones tests pass on Oracle also without that commit.

I noticed that the __search lookup isn't tested at all. It is only supported on MySQL. It likely doesn't work currently, but that is food for another ticket.

Edit note: One reason for the high amount of regressions is that I intentionally did not try to move existing make_atom() logic to class based lookups. The make_atom logic was somewhat weird in some places. The end result should be much cleaner now.

The __search lookup indeed doesn't work. I've opened a ticked & pull request: #22489

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