dexter has uploaded this change for review.

View Change

card_key_provider: refactor code and optimize out get_field method

The method get_field in the base class can be optimized out. This
also allows us to remove code dup in the card_key_provider_get_field
function.

Let's also fix the return code behavior. A get method in a
CardKeyProvider implementation should always return None in case
nothing is found. Also it should not crash in that case. This will
allow the card_key_provider_get function to move on to the next
CardKeyProvider. In case no CardKeyProvider yields any results, an
exception is appropriate since it is pointless to continue execution
with "None" as key material.

To make the debugging of problems easier, let's also print some debug
messages that inform the user what key/value pair and which
CardKeyProvider was queried. This will make it easier to investigate
in case an expected result was not found.

Related: SYS#7725
Change-Id: I4d6367b8eb057e7b2c06c8625094d8a1e4c8eef9
---
M pySim/card_key_provider.py
1 file changed, 25 insertions(+), 24 deletions(-)

git pull ssh://gerrit.osmocom.org:29418/pysim refs/changes/81/41481/1

diff --git a/pySim/card_key_provider.py b/pySim/card_key_provider.py
index cdd871f..b8578fa 100644
--- a/pySim/card_key_provider.py
+++ b/pySim/card_key_provider.py
@@ -132,24 +132,24 @@
 class CardKeyProvider(abc.ABC):
     """Base class, not containing any concrete implementation."""
 
-    def get_field(self, field: str, key: str = 'ICCID', value: str = "") -> Optional[str]:
-        """get a single field from CSV file using a specified key/value pair"""
-        fields = [field]
-        result = self.get(fields, key, value)
-        return result.get(field)
-
     @abc.abstractmethod
     def get(self, fields: List[str], key: str, value: str) -> Dict[str, str]:
-        """Get multiple card-individual fields for identified card.
+        """
+        Get multiple card-individual fields for identified card. This method should not fail with an exception in
+        case the entry, columns or even the key column itsself is not found.
 
         Args:
                 fields : list of valid field names such as 'ADM1', 'PIN1', ... which are to be obtained
                 key : look-up key to identify card data, such as 'ICCID'
                 value : value for look-up key to identify card data
         Returns:
-                dictionary of {field, value} strings for each requested field from 'fields'
+                dictionary of {field : value, ...} strings for each requested field from 'fields'. In case nothing is
+                fond None shall be returned.
         """
 
+    def __str__(self):
+        return type(self).__name__
+
 class CardKeyProviderCsv(CardKeyProvider):
     """Card key provider implementation that allows to query against a specified CSV file."""
 
@@ -172,15 +172,20 @@
             raise RuntimeError("Could not open DictReader for CSV-File '%s'" % self.csv_filename)
         cr.fieldnames = [field.upper() for field in cr.fieldnames]
 
-        rc = {}
+        if key not in cr.fieldnames:
+            return None
+        return_dict = {}
         for row in cr:
             if row[key] == value:
                 for f in fields:
                     if f in row:
-                        rc.update({f: self.crypt.decrypt_field(f, row[f])})
+                        return_dict.update({f: self.crypt.decrypt_field(f, row[f])})
                     else:
                         raise RuntimeError("CSV-File '%s' lacks column '%s'" % (self.csv_filename, f))
-        return rc
+        if return_dict == {}:
+            return None
+        return return_dict
+
 
 
 def card_key_provider_register(provider: CardKeyProvider, provider_list=card_key_providers):
@@ -210,12 +215,14 @@
     fields = [f.upper() for f in fields]
     for p in provider_list:
         if not isinstance(p, CardKeyProvider):
-            raise ValueError(
-                "provider list contains element which is not a card data provider")
+            raise ValueError("Provider list contains element which is not a card data provider")
+        log.debug("Searching for card key data (key=%s, value=%s, provider=%s)" % (key, value, str(p)))
         result = p.get(fields, key, value)
         if result:
+            log.debug("Found card data: %s" % (str(result)))
             return result
-    return {}
+
+    raise ValueError("Unable to find card key data (key=%s, value=%s, fields=%s)" % (key, value, str(fields)))
 
 
 def card_key_provider_get_field(field: str, key: str, value: str, provider_list=card_key_providers) -> Optional[str]:
@@ -229,13 +236,7 @@
     Returns:
             dictionary of {field, value} strings for the requested field
     """
-    key = key.upper()
-    field = field.upper()
-    for p in provider_list:
-        if not isinstance(p, CardKeyProvider):
-            raise ValueError(
-                "provider list contains element which is not a card data provider")
-        result = p.get_field(field, key, value)
-        if result:
-            return result
-    return None
+
+    fields = [field]
+    result = card_key_provider_get(fields, key, value, card_key_providers)
+    return result.get(field.upper())

To view, visit change 41481. To unsubscribe, or for help writing mail filters, visit settings.