remove inappropriate assert() usage

[Alexander Gorodnev]

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)

[John Bresnahan (jbresnah)]

Asserts should really only be used in tests. However, if there is a place in the code where you know a given state should not be possible but perhaps could happen (for example memory corruption) and you want to trap it in a known place instead of letting it float through the system in less detectable places assert can be used. It should be considered a debugging tool and not a way to validate parameters.

Below are some places this exists in glance. Each should be validated for proper assertion use.

https://github.com/openstack/glance/blob/master/glance/api/v2/images.py#L110
https://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L84
https://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L88
https://github.com/openstack/glance/blob/master/glance/common/rpc.py#L97
https://github.com/openstack/glance/blob/master/glance/db/__init__.py#L58
https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py#L94
https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py#L157
https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/api.py#L154
https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/api.py#L212
https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/api.py#L456
https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/migration.py#L134

[Kun Huang (academicgareth)]

totally agreed

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[Salvatore Orlando (salvatore-orlando)]

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

[OpenStack Infra (hudson-openstack)]

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

[Mark McLoughlin (markmc)]

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

[OpenStack Infra (hudson-openstack)]

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

[Ian Cordasco (icordasc)]

The following should still be reviewed:

https://github.com/openstack/glance/blob/9622189e728468e0cd8b358d1ebcb417f16d449f/glance/api/policy.py#L321

https://github.com/openstack/glance/blob/9622189e728468e0cd8b358d1ebcb417f16d449f/glance/common/rpc.py#L123

https://github.com/openstack/glance/blob/9622189e728468e0cd8b358d1ebcb417f16d449f/glance/db/__init__.py#L68

(The other two occurrences are in glance/tests/, so those are okay as is.)

[OpenStack Infra (hudson-openstack)]

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