remove inappropriate assert() usage

Bug #1199354 reported by Alexander Gorodnev on 2013-07-09
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Low
Kun Huang
Glance
Low
Unassigned
OpenStack Compute (nova)
Low
Kun Huang
neutron
Undecided
Hua Zhang

Bug Description

There are few places in nova where asserts are used in inappropriate manner. The example is below.

    for option in ['memory_mb', 'vcpus']:
        try:
            assert int(str(kwargs[option])) > 0
            kwargs[option] = int(kwargs[option])
        except (ValueError, AssertionError, TypeError):
            msg = _("'%s' argument must be a positive integer") % option
            raise exception.InvalidInput(reason=msg)

Changed in nova:
assignee: nobody → Alexander Gorodnev (a-gorodnev)
affects: nova → cinder
John Bresnahan (jbresnah) wrote :
Kun Huang (academicgareth) wrote :

totally agreed

Haomai Wang (haomai) on 2013-07-10
Changed in nova:
assignee: nobody → Haomai Wang (haomai)
Changed in glance:
assignee: nobody → Haomai Wang (haomai)
assignee: Haomai Wang (haomai) → nobody
Changed in glance:
assignee: nobody → Alexander Gorodnev (a-gorodnev)
Changed in cinder:
status: New → Confirmed
importance: Undecided → Low
Changed in cinder:
assignee: Alexander Gorodnev (a-gorodnev) → Kun Huang (academicgareth)
status: Confirmed → In Progress

Fix proposed to branch: master
Review: https://review.openstack.org/36603

Changed in nova:
status: New → In Progress
Hua Zhang (zhhuabj) on 2013-07-12
Changed in neutron:
assignee: nobody → Hua Zhang (zhhuabj)

Fix proposed to branch: master
Review: https://review.openstack.org/36800

Changed in neutron:
status: New → In Progress
Changed in glance:
status: New → In Progress

I respectfully disagree with the claim that assert should be used only in tests.
I agree they should not be used for input validation though.

My reference for assert usage is this: http://wiki.python.org/moin/UsingAssertionsEffectively

Reviewed: https://review.openstack.org/36602
Committed: http://github.com/openstack/cinder/commit/718a5293bc9ac36f5cbccb35296322fd7446f36d
Submitter: Jenkins
Branch: master

commit 718a5293bc9ac36f5cbccb35296322fd7446f36d
Author: Kun Huang <email address hidden>
Date: Thu Jul 11 12:04:52 2013 +0800

    remove improper assert usage

    There're many talks about it. An assert should be used for `never
    happen` cases, not common paramaters validating.

    With grep, we could many all assert statement used in none-test codes:

    cinder/volume/drivers/san/solaris.py:110:
    cinder/volume/drivers/san/solaris.py:116:
    cinder/volume/drivers/san/solaris.py:161:
    cinder/volume/drivers/san/solaris.py:162:
    cinder/volume/drivers/san/solaris.py:163:
    cinder/volume/drivers/san/solaris.py:164:
    cinder/volume/drivers/san/solaris.py:170:
        checking cmd output which should never changed, so leave it
    cinder/db/sqlalchemy/migration.py:113:
        ensure file existence from impossible cases, so leave it
    cinder/utils.py:
        used for functional flow, so use ValueError instead

    fixes bug #1199354
    Change-Id: I2b1701269bdf7c8737548e57bd940921a6256372

Changed in cinder:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2013-07-17
Changed in cinder:
milestone: none → havana-2
status: Fix Committed → Fix Released
Sean Dague (sdague) on 2013-07-19
Changed in nova:
importance: Undecided → Low
Changed in neutron:
status: In Progress → Won't Fix
Changed in nova:
assignee: Haomai Wang (haomai) → Kun Huang (academicgareth)
Mark McLoughlin (markmc) wrote :

Note - I do think there are some cases where using assert outside of tests makes sense

see e.g. https://review.openstack.org/#/c/36603/13/nova/api/ec2/cloud.py

Reviewed: https://review.openstack.org/40094
Committed: http://github.com/openstack/nova/commit/6554c47896c1306b6abdbcc7f54495c8edfa9813
Submitter: Jenkins
Branch: master

commit 6554c47896c1306b6abdbcc7f54495c8edfa9813
Author: Kun Huang <email address hidden>
Date: Sun Aug 4 16:47:16 2013 +0800

    remove improper usage of 'assert'

    The assert will not raise AssertionError when run Python with '-O' flag.
    And at the same time, any program should have a same result with or
    without '-O' flag. So we could use assert to check some rare condition,
    but we couldn't use it in logic flow or except it must raise an
    Assertionerror, for example:

    this is not bad:

    def func(fname):
        assert os.path.exist(fname)
        f = open(fname)
        ...

    but this will have different results with or without '-O' flag

    def func(num):
        try:
            assert num is 5
        except: Assertionerror:
            log.error("your number isn't 5!")

    The second case is what I fixes in this patch.

    fixes bug #1199354
    Change-Id: I20e83ecfc181d213f357aa74e0af3a110526904c

Changed in nova:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2013-09-05
Changed in nova:
milestone: none → havana-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2013-10-17
Changed in cinder:
milestone: havana-2 → 2013.2
Thierry Carrez (ttx) on 2013-10-17
Changed in nova:
milestone: havana-3 → 2013.2
Erno Kuvaja (jokke) on 2014-09-12
Changed in glance:
assignee: Alexander Gorodnev (a-gorodnev) → nobody
status: In Progress → New
tags: added: propose-close
Ian Cordasco (icordasc) wrote :
tags: added: low-hanging-fruit
Changed in glance:
importance: Undecided → Low
status: New → Triaged
Changed in glance:
assignee: nobody → Cindy Pallares (cindy-pallaresq)

Reviewed: https://review.openstack.org/156348
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=f1d094021ddd10d8752d3463a8a8c189563dd93c
Submitter: Jenkins
Branch: master

commit f1d094021ddd10d8752d3463a8a8c189563dd93c
Author: Kasey Alusi <email address hidden>
Date: Mon Feb 16 11:46:04 2015 -0800

    Removes unnecessary assert

        Three suggested asserts to examine:
        - glance/api/policy.py
            removed unnecesary assert from _get_checker private method
        - glace/common/rpc.py
            asserts parameter is callable, left it
        - glance/db/__init__.py
            asserts that retrieved image is not deleted, left it

    Change-Id: I7dfc30fe4a93ce1e96888467bdd47768222f98a5
    Closes-Bug: #1199354

Changed in glance:
status: Triaged → Fix Committed
Changed in glance:
assignee: Cindy Pallares (cindy-pallaresq) → nobody
Thierry Carrez (ttx) on 2015-03-19
Changed in glance:
milestone: none → kilo-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2015-04-30
Changed in glance:
milestone: kilo-3 → 2015.1.0
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers