Refactor Drive service into a class #489
Conversation
Apophenia
commented
Dec 20, 2024
•
Apophenia
commented
- Refactor drive service into a class
- Refactor SSM service into a class
- Add static method for parsing Google-specific URLs
- SSM service returns parameter only as string instead of parameter object from boto3 for better parity with environment variable usage
- Add test environment variable to test helpers
Refactor drive service into a class & only return param from SSM service
| @@ -265,7 +274,7 @@ def test_parseEditionToPublication_ReaderFlagTrue(self, testPubEls, mocker): | |||
| mocker.call('rights', {'source': 'testSource', 'license': 'testLicense', 'rightsStatement': 'testStatement'}) | |||
| ]) | |||
|
|
|||
| pubMocks['addLink'].assert_called_with({'href': 'https://digital-research-books-beta.nypl.org/read/testID', 'type': 'testType', 'rel': 'http://opds-spec.org/acquisition/open-access', 'identifier': 'readable'}) | |||
| pubMocks['addLink'].assert_called_with({'href': 'https://drb-qa.nypl.org/read/testID', 'type': 'testType', 'rel': 'http://opds-spec.org/acquisition/open-access', 'identifier': 'readable'}) | |||
There was a problem hiding this comment.
The environment variable used by this function seemed to be different when I ran the tests individually vs as a set, and then seemed to also behave different on my machine vs QA. I wondered if it was being set by TestFlaskAPI (which manually sets it to production) in some cases, but it was cleared to an empty string (perhaps by clearEnvVars) in others. In any case, adding the helper setup and teardown seemed to normalize it to a "test" environment.
I'm making a note to investigate the state sharing across unit tests as part of a test audit.
| if os.environ['ENVIRONMENT'] == 'production': | ||
| SERVICE_ACCOUNT_FILE = ssm_service.get_parameter('arn:aws:ssm:us-east-1:946183545209:parameter/drb/production/google-drive-service-key') | ||
| else: | ||
| SERVICE_ACCOUNT_FILE = ssm_service.get_parameter('arn:aws:ssm:us-east-1:946183545209:parameter/drb/qa/google-drive-service-key') |
There was a problem hiding this comment.
Would it be better to encapsulate the ARNs in e.g. a dictionary within the SSM service itself?
There was a problem hiding this comment.
could you dossm_service.get_parameter(f'arn.../{os.environ.get('ENVIRONMENT', 'qa')}... to make this easier?
There was a problem hiding this comment.
It would definitely look better, but in some cases that variable would be 'test' or 'local' and wouldn't result in a valid ARN path - as it stands now, it falls back to using QA params locally.
As I'm testing the Airtable changes I've been using the local keys as a fallback anyway so that I can test against remote test data if I want to, but if I committed something like that it would be even more complex.
| return file | ||
|
|
||
| def get_file_metadata(self, file_id: str) -> Optional[str]: | ||
| # supportsAllDrives=True required as of 12/24 despite deprecation |
There was a problem hiding this comment.
is this still needed?
There was a problem hiding this comment.
I wanted to leave this here in case anyone tried to refactor this code and referred to the Google documentation, which is in some places incorrect. It looks like they added warnings to the codebase that the supportsAllDrives would be considered true by default after 2020, and planned to support Team drives by default, but this isn't the case at least for get_media().
There was a problem hiding this comment.
I lost a lot of time to this before I just tried adding it anyway - I didn't want the same to happen to anyone else.
services/google_drive_service.py
Outdated
| def get_file_metadata(self, file_id: str) -> Optional[str]: | ||
| # supportsAllDrives=True required as of 12/24 despite deprecation | ||
| request = self.drive_service.files().get(fileId=file_id, supportsAllDrives=True) | ||
| meta = request.execute() |
There was a problem hiding this comment.
what is meta? the variable name isn't quite clear to me - sorry!
