yahoo-eng-team team mailing list archive

[Brian Rosmaita <brian.rosmaita@xxxxxxxxxxxxx>]

Public bug reported:

Apologies in advance for what's going to be a very long story for a
simple change in nova/utils.py.

Change b7e9a64416ff239a4c1b8501f398796b02c46ce7 introduces a
filter_deleted flag into the metadata_to_dict function.   The name
implies (to me anyway) that when the flag is False (as it is by
default), deleted items will NOT be filtered out of the dict returned by
the function.  However, that's not what happens: when
filter_deleted=False, deleted items ARE excluded.

Point 1: this is a naming issue, not a code issue.  Here's what we've got:
Original code: returns a dict of the non-deleted metadata.

def metadata_to_dict(metadata):
    result = {}
    for item in metadata:
    	if not item.get('deleted'):
	   result[item['key']] = item['value']
   return result

Current code: adds a new parameter with a default value ... so you'd
expect the behavior with the default parameter setting to be the same as
the original function.

def metadata_to_dict(metadata, filter_deleted=False):
    result = {}
    for item in metadata:
        if not filter_deleted and item.get('deleted'):
            continue
        result[item['key']] = item['value']
    return result

Summary of Point 1:
If the default value is used, deleted items will not be included in the returned dict, so original behavior is preserved.
If filter_deleted=True is passed to the function, the 'if' will fail and deleted metadata will be included, which is new behavior.
Hence, the code is OK ... but it would be much clearer if the parameter were named 'include_deleted'.  See the next point for why this matters.

Point 2: the naming issue is important.

Three utility functions call the metadata_to_dict function:
(a) def instance_meta(instance) -> calls without setting the parameter, so no change in behavior: Correct
(b) def instance_sys_meta(instance) -> sets filter_deleted=True -> change in behavior (now includes deleted instance_system_metadata)
(c) def get_image_from_system_metadata(system_meta) -> sets filter_deleted=True -> change in behavior (now includes deleted instance_system_metadata)

The change in behavior for (b) and (c) is a breaking change for a
utility function, which is a bad practice.  I think what's going on is
that when the functions were changed to use the new function signature
for metadata_to_dict(), filter_deleted was set to True so that the
functions would filter out deleted stuff, thereby preserving their
original behavior.  But the opposite happened, namely, now they include
deleted stuff.  Actually, the intention doesn't matter here--a breaking
change to a utility function should not occur.  My real point is that
when you look at functions (b) and (c) in isolation, filter_deleted=True
appears to be what you'd want to pass to metadata_to_dict, whereas you
need to pass filter_deleted=False to get the correct behavior.

This bug:
All I want to do is rename filter_deleted in the function metadata_to_dict() to 'include_deleted'.  (I'll file a separate bug to correct the utility functions that use the metadata_to_dict() function.)

References:
The function is here: https://github.com/openstack/nova/blob/a2d5492e8a15cdc13ada61b03f6293c709160505/nova/utils.py#L847-L853
The change was introduced by https://review.openstack.org/#/c/109201/
The change is b7e9a64416ff239a4c1b8501f398796b02c46ce7

** Affects: nova
     Importance: Undecided
         Status: New

-- 
You received this bug notification because you are a member of Yahoo!
Engineering Team, which is subscribed to OpenStack Compute (nova).
https://bugs.launchpad.net/bugs/1496394

Title:
  misleadingly named parameter in metadata_to_dict utility function

Status in OpenStack Compute (nova):
  New

Bug description:
  Apologies in advance for what's going to be a very long story for a
  simple change in nova/utils.py.

  Change b7e9a64416ff239a4c1b8501f398796b02c46ce7 introduces a
  filter_deleted flag into the metadata_to_dict function.   The name
  implies (to me anyway) that when the flag is False (as it is by
  default), deleted items will NOT be filtered out of the dict returned
  by the function.  However, that's not what happens: when
  filter_deleted=False, deleted items ARE excluded.

  Point 1: this is a naming issue, not a code issue.  Here's what we've got:
  Original code: returns a dict of the non-deleted metadata.

  def metadata_to_dict(metadata):
      result = {}
      for item in metadata:
      	if not item.get('deleted'):
  	   result[item['key']] = item['value']
     return result

  Current code: adds a new parameter with a default value ... so you'd
  expect the behavior with the default parameter setting to be the same
  as the original function.

  def metadata_to_dict(metadata, filter_deleted=False):
      result = {}
      for item in metadata:
          if not filter_deleted and item.get('deleted'):
              continue
          result[item['key']] = item['value']
      return result

  Summary of Point 1:
  If the default value is used, deleted items will not be included in the returned dict, so original behavior is preserved.
  If filter_deleted=True is passed to the function, the 'if' will fail and deleted metadata will be included, which is new behavior.
  Hence, the code is OK ... but it would be much clearer if the parameter were named 'include_deleted'.  See the next point for why this matters.

  Point 2: the naming issue is important.

  Three utility functions call the metadata_to_dict function:
  (a) def instance_meta(instance) -> calls without setting the parameter, so no change in behavior: Correct
  (b) def instance_sys_meta(instance) -> sets filter_deleted=True -> change in behavior (now includes deleted instance_system_metadata)
  (c) def get_image_from_system_metadata(system_meta) -> sets filter_deleted=True -> change in behavior (now includes deleted instance_system_metadata)

  The change in behavior for (b) and (c) is a breaking change for a
  utility function, which is a bad practice.  I think what's going on is
  that when the functions were changed to use the new function signature
  for metadata_to_dict(), filter_deleted was set to True so that the
  functions would filter out deleted stuff, thereby preserving their
  original behavior.  But the opposite happened, namely, now they
  include deleted stuff.  Actually, the intention doesn't matter here--a
  breaking change to a utility function should not occur.  My real point
  is that when you look at functions (b) and (c) in isolation,
  filter_deleted=True appears to be what you'd want to pass to
  metadata_to_dict, whereas you need to pass filter_deleted=False to get
  the correct behavior.

  This bug:
  All I want to do is rename filter_deleted in the function metadata_to_dict() to 'include_deleted'.  (I'll file a separate bug to correct the utility functions that use the metadata_to_dict() function.)

  References:
  The function is here: https://github.com/openstack/nova/blob/a2d5492e8a15cdc13ada61b03f6293c709160505/nova/utils.py#L847-L853
  The change was introduced by https://review.openstack.org/#/c/109201/
  The change is b7e9a64416ff239a4c1b8501f398796b02c46ce7

To manage notifications about this bug go to:
https://bugs.launchpad.net/nova/+bug/1496394/+subscriptions