Update all fields on AdvisorData upsert (#155)

* update all fields on upsert

* add a test for zeroing out lastAuthenticated, silence logs in tests, rename fixtures

* add docstrings to fixtures

* post-rebase fixes

aardvark/app.py

@@ -1,13 +1,19 @@
+from __future__ import annotations
+
 import os.path
 import logging
+import sys
 from logging import DEBUG, Formatter, StreamHandler
 from logging.config import dictConfig
-import sys
+from typing import TYPE_CHECKING
 
 from flask_sqlalchemy import SQLAlchemy
 from flask import Flask
 from flasgger import Swagger
 
+if TYPE_CHECKING:
+    from flask import Config
+
 db = SQLAlchemy()
 
 from aardvark.view import mod as advisor_bp  # noqa
@@ -19,16 +25,19 @@
 API_VERSION = '1'
 
 
-def create_app():
+def create_app(config_override: Config = None):
     app = Flask(__name__, static_url_path='/static')
     Swagger(app)
 
-    path = _find_config()
-    if not path:
-        print('No config')
-        app.config.from_pyfile('_config.py')
+    if config_override:
+        app.config.from_mapping(config_override)
     else:
-        app.config.from_pyfile(path)
+        path = _find_config()
+        if not path:
+            print('No config')
+            app.config.from_pyfile('_config.py')
+        else:
+            app.config.from_pyfile(path)
 
     # For ELB and/or Eureka
     @app.route('/healthcheck')

aardvark/model.py

@@ -1,3 +1,5 @@
+from __future__ import annotations
+
 import datetime
 
 from flask import current_app
@@ -69,50 +71,68 @@ class AdvisorData(db.Model):
     @staticmethod
     def create_or_update(item_id, lastAuthenticated, serviceName, serviceNamespace, lastAuthenticatedEntity,
                          totalAuthenticatedEntities):
+        # Truncate service name and namespace to make sure they fit in our DB fields
         serviceName = serviceName[:128]
         serviceNamespace = serviceNamespace[:64]
-        item = None
+
+        # Query the database for an existing entry that matches this item ID and service namespace. If there is none,
+        # instantiate an empty AdvisorData
+        item: AdvisorData | None = None
         try:
-            item = AdvisorData.query.filter(AdvisorData.item_id == item_id).filter(AdvisorData.serviceNamespace ==
-                                                                                   serviceNamespace).scalar()
+            item = db.session.query(AdvisorData).filter(
+                AdvisorData.item_id == item_id,
+                AdvisorData.serviceNamespace == serviceNamespace,
+            ).scalar()
         except sqlalchemy.exc.SQLAlchemyError as e:
-            current_app.logger.error('Database error: {} item_id: {} serviceNamespace: {}'.format(e.args[0], item_id,
-                                     serviceNamespace)) #exception.messsage not supported in py3 e.args[0] replacement
+            current_app.logger.error(
+                'Database error: %s item_id: %s serviceNamespace: %s',
+                str(e),
+                item_id,
+                serviceNamespace
+            )
 
         if not item:
-            item = AdvisorData(item_id=item_id,
-                               lastAuthenticated=lastAuthenticated,
-                               serviceName=serviceName,
-                               serviceNamespace=serviceNamespace,
-                               lastAuthenticatedEntity=lastAuthenticatedEntity,
-                               totalAuthenticatedEntities=totalAuthenticatedEntities)
-            db.session.add(item)
-            return
-
-        if lastAuthenticated > item.lastAuthenticated:
-            item.lastAuthenticated = lastAuthenticated
-            db.session.add(item)
-
-        elif lastAuthenticated < item.lastAuthenticated:
-            """
-            lastAuthenticated is obtained by calling get_service_last_accessed_details() method of the boto3 iam client.
-            When there is no AA data about a service, the lastAuthenticated key is missing from the returned dictionary.
-            This is perfectly valid, either because the service in question was not accessed in the past 365 days or
-            the entity granting  access to it was created recently enough that no AA data is available yet (it can take up to
-            4 hours for this to happen).
-            When this happens, the AccountToUpdate._get_job_results() method will set lastAuthenticated to 0.
-            Usually we don't want to persist such an entity, with one exception: there's already a recorded, non-zero lastAuthenticated
-            timestamp persisted for this item. That means the service was accessed at some point in time, but now more than 365 passed since
-            the last access, so AA no longer returns a timestamp for it.
-            """
+            item = AdvisorData()
+
+        # Save existing lastAuthenticated timestamp for later comparison
+        existingLastAuthenticated = item.lastAuthenticated or 0
+
+        # Set all fields to the provided values. SQLAlchemy will only mark the model instance as modified if the actual
+        # values have changed, so this will be a no-op if the values are all the same.
+        item.item_id = item_id
+        item.lastAuthenticated = lastAuthenticated
+        item.lastAuthenticatedEntity = lastAuthenticatedEntity
+        item.serviceName = serviceName
+        item.serviceNamespace = serviceNamespace
+        item.totalAuthenticatedEntities = totalAuthenticatedEntities
+
+        # When there is no AA data about a service, the lastAuthenticated key is missing from the returned dictionary.
+        # This is perfectly valid, either because the service in question was not accessed in the past 365 days or
+        # the entity granting  access to it was created recently enough that no AA data is available yet (it can take
+        # up to 4 hours for this to happen).
+        #
+        # When this happens, the AccountToUpdate._get_job_results() method will set lastAuthenticated to 0. Usually
+        # we don't want to persist such an entity, with one exception: there's already a recorded, non-zero
+        # lastAuthenticated timestamp persisted for this item. That means the service was accessed at some point in
+        # time, but now more than 365 passed since the last access, so AA no longer returns a timestamp for it.
+        if lastAuthenticated < existingLastAuthenticated:
             if lastAuthenticated == 0:
-                current_app.logger.warn('Previously seen object not accessed in the past 365 days '
-                                        '(got null lastAuthenticated from AA). Setting to 0. '
-                                        'Object {} service {} previous timestamp {}'.format(item.item_id, item.serviceName, item.lastAuthenticated))
-                item.lastAuthenticated = 0
-                db.session.add(item)
+                current_app.logger.info(
+                    'Previously seen object not accessed in the past 365 days (got null lastAuthenticated from AA). '
+                    'Setting to 0. Object %s service %s previous timestamp %d',
+                    item.item_id,
+                    item.serviceName,
+                    item.lastAuthenticated
+                )
             else:
-                current_app.logger.error("Received an older time than previously seen for object {} service {} ({la} < {ila})!".format(item.item_id,
-                                                                                                                                       item.serviceName,
-                                                                                                                                       la=lastAuthenticated,
-                                                                                                                                       ila=item.lastAuthenticated))
+                current_app.logger.warning(
+                    "Received an older time than previously seen for object %s service %s (%d < %d)!",
+                    item.item_id,
+                    item.serviceName,
+                    lastAuthenticated,
+                    existingLastAuthenticated
+                )
+                item.lastAuthenticated = existingLastAuthenticated
+
+        # Add the updated item to the session so it gets committed with everything else
+        db.session.add(item)

test/conftest.py

@@ -0,0 +1,34 @@
+import pytest
+from flask_sqlalchemy import SQLAlchemy
+
+
+@pytest.fixture(scope="function")
+def app_config():
+    """Return a Flask configuration object for testing. The returned configuration is intended to be a good base for
+    testing and can be customized for specific testing needs.
+    """
+    from flask import Config
+    c = Config('.')
+    c.from_mapping({
+        "SQLALCHEMY_DATABASE_URI": "sqlite:///:memory:",
+        "SQLALCHEMY_TRACK_MODIFICATIONS": False,
+        "DEBUG": False,
+        "LOG_CFG": {'version': 1, 'handlers': []},  # silence logging
+    })
+    return c
+
+
+@pytest.fixture(scope="function")
+def mock_database(app_config):
+    """Yield an instance of flask_sqlalchemy.SQLAlchemy associated with the base model class used in aardvark.model.
+    This is almost certainly not safe for parallel/multi-threaded use.
+    """
+    from aardvark.app import db
+    mock_db = SQLAlchemy(model_class=db.Model)
+
+    from aardvark.app import create_app
+    app = create_app(config_override=app_config)
+    with app.app_context():
+        mock_db.create_all()
+        yield mock_db
+        mock_db.drop_all()

test/test_model.py

@@ -0,0 +1,146 @@
+def test_advisor_data_create(mock_database):
+    from aardvark.model import AdvisorData, db
+    AdvisorData.create_or_update(
+        9999,
+        1111,
+        "Swifties United",
+        "swift",
+        "taylor",
+        1,
+    )
+    db.session.commit()
+
+    record: AdvisorData = AdvisorData.query.filter(
+        AdvisorData.item_id == 9999,
+        AdvisorData.serviceNamespace == "swift",
+    ).scalar()
+    assert record
+    assert record.id
+    assert record.item_id == 9999
+    assert record.lastAuthenticated == 1111
+    assert record.lastAuthenticatedEntity == "taylor"
+    assert record.serviceName == "Swifties United"
+    assert record.serviceNamespace == "swift"
+    assert record.totalAuthenticatedEntities == 1
+
+
+def test_advisor_data_update(mock_database):
+    from aardvark.model import AdvisorData, db
+    AdvisorData.create_or_update(
+        9999,
+        0,
+        "Pink Pony Club",
+        "roan",
+        None,
+        0,
+    )
+    db.session.commit()
+
+    record: AdvisorData = db.session.query(AdvisorData).filter(
+        AdvisorData.id == 1,
+    ).scalar()
+    assert record
+    assert record.lastAuthenticated == 0
+
+    AdvisorData.create_or_update(
+        9999,
+        1111,
+        "Pink Pony Club",
+        "roan",
+        "chappell",
+        1,
+    )
+    db.session.commit()
+
+    record: AdvisorData = db.session.query(AdvisorData).filter(
+        AdvisorData.id == 1,
+    ).scalar()
+    assert record
+    assert record.item_id == 9999
+    assert record.lastAuthenticated == 1111
+    assert record.lastAuthenticatedEntity == "chappell"
+    assert record.serviceName == "Pink Pony Club"
+    assert record.serviceNamespace == "roan"
+    assert record.totalAuthenticatedEntities == 1
+
+
+def test_advisor_data_update_older_last_authenticated(mock_database):
+    from aardvark.model import AdvisorData, db
+    AdvisorData.create_or_update(
+        9999,
+        1111,
+        "Pink Pony Club",
+        "roan",
+        "chappell",
+        1,
+    )
+    db.session.commit()
+
+    record: AdvisorData = db.session.query(AdvisorData).filter(
+        AdvisorData.id == 1,
+        ).scalar()
+    assert record
+    assert record.lastAuthenticated == 1111
+
+    # Calling create_or_update with a lower lastAuthenticated value should NOT update lastAuthenticated in the DB
+    AdvisorData.create_or_update(
+        9999,
+        1000,
+        "Pink Pony Club",
+        "roan",
+        "chappell",
+        1,
+    )
+    db.session.commit()
+
+    record: AdvisorData = db.session.query(AdvisorData).filter(
+        AdvisorData.id == 1,
+        ).scalar()
+    assert record
+    assert record.item_id == 9999
+    assert record.lastAuthenticated == 1111
+    assert record.lastAuthenticatedEntity == "chappell"
+    assert record.serviceName == "Pink Pony Club"
+    assert record.serviceNamespace == "roan"
+    assert record.totalAuthenticatedEntities == 1
+
+
+def test_advisor_data_update_zero_last_authenticated(mock_database):
+    from aardvark.model import AdvisorData, db
+    AdvisorData.create_or_update(
+        9999,
+        1111,
+        "Pink Pony Club",
+        "roan",
+        "chappell",
+        1,
+    )
+    db.session.commit()
+
+    record: AdvisorData = db.session.query(AdvisorData).filter(
+        AdvisorData.id == 1,
+        ).scalar()
+    assert record
+    assert record.lastAuthenticated == 1111
+
+    # Calling create_or_update with a zero lastAuthenticated value SHOULD update lastAuthenticated in the DB
+    AdvisorData.create_or_update(
+        9999,
+        0,
+        "Pink Pony Club",
+        "roan",
+        "",
+        0,
+    )
+    db.session.commit()
+
+    record: AdvisorData = db.session.query(AdvisorData).filter(
+        AdvisorData.id == 1,
+        ).scalar()
+    assert record
+    assert record.item_id == 9999
+    assert record.lastAuthenticated == 0
+    assert record.lastAuthenticatedEntity == ""
+    assert record.serviceName == "Pink Pony Club"
+    assert record.serviceNamespace == "roan"
+    assert record.totalAuthenticatedEntities == 0