Spike on a full redesign of the client #326

mjgiarlo · 2024-01-12T23:58:39Z

Why was this change made?

Connects to #261

This commit adds a new public interface for sdr-client to see if we can make a less complex interaction with the client workable. See #261 for more rationale and analysis.

The work in this PR is only the bits of sdr-client that google-books and the sdr-client CLI depends on as a start, with some changes likely needed for Argo, H2, and the infra-integration-tests.

Note: the intent of this PR is not to rewrite and refactor the entire world, but to do sufficient internal refactoring to create a new seam (the public interface) to enable future internal refactoring. Until we move to a narrower interface, sdr-client refactors are extremely difficult to do.

Goals of the re-design include:

Provide an interface for handling operations that rely on asynchronous interactions (ones that rely on SDR API background jobs)
Build in smart token refreshing
Keep the client interface as tidy as can be defended instead of expecting consumers to knit together implementation details
Do this in a way that does not break any current interfaces, and allows us to move over to the new interfaces gradually at our own pace

Why do this at all? We've learned lessons about client gem design over the past couple years and sdr-client could use some refreshing based on these lessons.

Ultimately the goal is to promote the new client, as implemented in the SdrClient::RedesignedClient module, to the main module namespace and delete the old code entirely. We can do that once we've moved over google-books, argo, H2, the integration-tests, and the SDR CLI to the new interface and tested it all in QA and stage.

How was this change tested?

CI
integration tests

lib/sdr-client.rb

mjgiarlo · 2024-01-13T00:21:54Z

lib/sdr_client.rb

+require 'zeitwerk'

-require 'sdr_client/version'
-require 'sdr_client/unexpected_response'
-require 'sdr_client/deposit'
-require 'sdr_client/update'
-require 'sdr_client/credentials'
-require 'sdr_client/find'
-require 'sdr_client/login'
-require 'sdr_client/login_prompt'
-require 'sdr_client/connection'
-require 'sdr_client/background_job_results'
+loader = Zeitwerk::Loader.for_gem
+loader.inflector.inflect(
+  'md5' => 'MD5',
+  'sha1' => 'SHA1'
+)
+loader.setup


Now we don't have to add manual require statements whenever we add a new class or rename a class to the client.

lib/sdr_client/redesigned_client.rb

mjgiarlo · 2024-01-13T00:23:17Z

lib/sdr_client/redesigned_client.rb

+    def with_token_refresh_when_unauthorized
+      response = yield
+
+      # if unauthorized, token has likely expired. try to get a new token and then retry the same request(s).
+      if response.status == 401
+        config.token = Authenticator.token
+        response = yield
+      end
+
+      response
+    end


Automagic token refreshing.

mjgiarlo · 2024-01-13T00:23:57Z

lib/sdr_client/redesigned_client/create_resource.rb

+      def logger
+        SdrClient::RedesignedClient.config.logger
+      end
+
+      def client
+        SdrClient::RedesignedClient.instance
+      end


Get the client instance and the logger from singleton config so we don't have to pass params around everywhere.

lib/sdr_client/redesigned_client/update_dro_with_file_identifiers.rb

mjgiarlo · 2024-01-13T00:25:54Z

lib/sdr_client/redesigned_client/job_status.rb

+
+module SdrClient
+  class RedesignedClient
+    # Wraps operations waiting for results from jobs


Deal with background/async interactions on behalf of consumers.

spec/spec_helper.rb

lib/sdr_client/redesigned_client.rb

mjgiarlo · 2024-03-27T18:08:10Z

exe/sdr_redesigned

+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+$LOAD_PATH.unshift 'lib'
+
+require 'sdr_client'
+require 'sdr_client/redesigned_client'
+require 'sdr_client/redesigned_client/cli'
+
+SdrClient::RedesignedClient::CLI.start(ARGV)


A little CLI wrapper that uses only classes in the redesigned module space. This lets us test the changes and make sure they function as expected in the CLI.

mjgiarlo · 2024-03-27T18:10:14Z

lib/sdr_client/redesigned_client/cli/credentials.rb

+module SdrClient
+  class RedesignedClient
+    class CLI < Thor
+      # The stored credentials
+      class Credentials


The credentials file is now only needed by the CLI since usage in applications will handle authorization via automagic token refreshes.

mjgiarlo · 2024-03-27T18:11:50Z

lib/sdr_client/redesigned_client/deposit.rb

+      def deposit_model # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
+        check_files_exist!
+        child_files_match! unless options[:request_builder]
+
+        file_metadata = UploadFilesMetadataBuilder.build(files: files, mime_types: mime_types, basepath: basepath)
+        upload_responses = UploadFiles.upload(file_metadata: file_metadata,
+                                              filepath_map: filepath_map)
+        if options[:request_builder]
+          @model = StructuralGrouper.group(
+            request_builder: options[:request_builder],
+            upload_responses: upload_responses,
+            grouping_strategy: options[:grouping_strategy],
+            file_set_strategy: options[:file_set_strategy]
+          )
+          child_files_match!
+        end
+
+        new_request_dro = UpdateDroWithFileIdentifiers.update(request_dro: model,
+                                                              upload_responses: upload_responses)
+        CreateResource.run(accession: accession,
+                           priority: options[:priority],
+                           assign_doi: options[:assign_doi],
+                           metadata: new_request_dro)
+      end
+


This method is a bit messy because it's taking the place of two separate classes in the old design. Further internal refactoring would be good after we switch to the new public interface.

mjgiarlo · 2024-03-27T18:13:27Z

lib/sdr_client/redesigned_client/request_builder.rb

+      def initialize(apo:, source_id:, **options)
+        @apo = apo
+        @source_id = source_id
+        @options = options


Move all non-required options to an options param instead of having a giant list of keyword params.

mjgiarlo · 2024-03-27T18:14:07Z

lib/sdr_client/redesigned_client/unexpected_response.rb

+module SdrClient
+  class RedesignedClient
+    # Handles unexpected responses
+    class UnexpectedResponse


Handle unexpected responses all in one spot.

Fixes #261

justinlittman

Ka-ching

mjgiarlo mentioned this pull request Jan 13, 2024

assess sdr-client refactor to be more similar to our other client gems #261

Open

mjgiarlo commented Jan 13, 2024

View reviewed changes

lib/sdr-client.rb Outdated Show resolved Hide resolved

mjgiarlo commented Jan 13, 2024

View reviewed changes

lib/sdr_client/redesigned_client.rb Outdated Show resolved Hide resolved

mjgiarlo commented Jan 13, 2024

View reviewed changes

lib/sdr_client/redesigned_client/update_dro_with_file_identifiers.rb Outdated Show resolved Hide resolved

mjgiarlo commented Jan 13, 2024

View reviewed changes

spec/spec_helper.rb Outdated Show resolved Hide resolved

ndushay reviewed Jan 16, 2024

View reviewed changes

lib/sdr_client/redesigned_client.rb Show resolved Hide resolved

mjgiarlo force-pushed the redesign-spike branch 20 times, most recently from 5268f30 to c63b37d Compare March 13, 2024 20:45

mjgiarlo force-pushed the redesign-spike branch from c3778e7 to 4436e07 Compare March 14, 2024 19:39

mjgiarlo force-pushed the redesign-spike branch 9 times, most recently from b341bc9 to e55cc9f Compare March 26, 2024 20:36

mjgiarlo mentioned this pull request Mar 26, 2024

Use zeitwerk to auto-load internal dependencies #338

Merged

2 tasks

mjgiarlo force-pushed the redesign-spike branch 2 times, most recently from 8399619 to 76c1b2f Compare March 27, 2024 18:07

mjgiarlo commented Mar 27, 2024

View reviewed changes

mjgiarlo force-pushed the redesign-spike branch from 76c1b2f to b93a91b Compare March 27, 2024 18:15

mjgiarlo marked this pull request as ready for review April 2, 2024 15:47

Spike on a full redesign of the client

f4070da

Fixes #261

mjgiarlo force-pushed the redesign-spike branch from b93a91b to f4070da Compare April 22, 2024 21:00

mjgiarlo added 3 commits April 22, 2024 14:21

Bump version to 2.13.0.beta1 to test refactoring changes

f11fc34

Make sure Zeitwerk ignores the CLI files and directories

580220f

Bump version to 2.13.0.beta2

e571da8

mjgiarlo force-pushed the redesign-spike branch from 1ab249f to e571da8 Compare April 22, 2024 21:50

justinlittman approved these changes Apr 22, 2024

View reviewed changes

aaron-collier approved these changes Apr 22, 2024

View reviewed changes

mjgiarlo merged commit e0f8019 into main Apr 22, 2024
5 checks passed

mjgiarlo deleted the redesign-spike branch April 22, 2024 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spike on a full redesign of the client #326

Spike on a full redesign of the client #326

mjgiarlo commented Jan 12, 2024 •

edited

mjgiarlo Jan 13, 2024

mjgiarlo Jan 13, 2024

mjgiarlo Jan 13, 2024 •

edited

mjgiarlo Jan 13, 2024 •

edited

mjgiarlo Mar 27, 2024 •

edited

mjgiarlo Mar 27, 2024

mjgiarlo Mar 27, 2024

mjgiarlo Mar 27, 2024

mjgiarlo Mar 27, 2024

justinlittman left a comment

Spike on a full redesign of the client #326

Spike on a full redesign of the client #326

Conversation

mjgiarlo commented Jan 12, 2024 • edited

Why was this change made?

How was this change tested?

mjgiarlo Jan 13, 2024

Choose a reason for hiding this comment

mjgiarlo Jan 13, 2024

Choose a reason for hiding this comment

mjgiarlo Jan 13, 2024 • edited

Choose a reason for hiding this comment

mjgiarlo Jan 13, 2024 • edited

Choose a reason for hiding this comment

mjgiarlo Mar 27, 2024 • edited

Choose a reason for hiding this comment

mjgiarlo Mar 27, 2024

Choose a reason for hiding this comment

mjgiarlo Mar 27, 2024

Choose a reason for hiding this comment

mjgiarlo Mar 27, 2024

Choose a reason for hiding this comment

mjgiarlo Mar 27, 2024

Choose a reason for hiding this comment

justinlittman left a comment

Choose a reason for hiding this comment

mjgiarlo commented Jan 12, 2024 •

edited

mjgiarlo Jan 13, 2024 •

edited

mjgiarlo Jan 13, 2024 •

edited

mjgiarlo Mar 27, 2024 •

edited