Register event callback

[drernie]

Description

Strawman proposal for how to use boto3 events to solve the problem of "include appropriate parameters when writing S3 objects"

TODO

• Unit tests
• Confirm that this change meets security best practices and does not violate the security model
• [ X] Documentation
  • Markdown docs for developers
• Changelog entry (skip if change is not significant to end users, e.g. docs only)

[codecov]

Codecov Report

Attention: Patch coverage is 92.61745% with 22 lines in your changes missing coverage. Please review.

Project coverage is 39.06%. Comparing base (009d617) to head (04cce4f).

Files with missing lines	Patch %	Lines
api/python/tests/integration/test_put_options.py	91.30%	20 Missing ⚠️
api/python/tests/test_event_handlers.py	95.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4281      +/-   ##
==========================================
+ Coverage   38.59%   39.06%   +0.46%     
==========================================
  Files         778      780       +2     
  Lines       34394    34691     +297     
  Branches     5430     5225     -205     
==========================================
+ Hits        13276    13552     +276     
- Misses      19939    20596     +657     
+ Partials     1179      543     -636     

Flag	Coverage Δ
api-python	91.41% <92.61%> (+0.06%)	⬆️
catalog	17.24% <ø> (ø)
lambda	91.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[drernie]

@sir-sigurd @nl0 The basic idea is:

1. Maintain a list of events and callbacks
2. Add those when creating a new client
3. Unset the clients when updating the list (by passing an event-name/callback pair)
4. Add a convenience method to update all (only) the write_options

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

api/python/quilt3/data_transfer.py:182

• The new method add_options_safely introduces new behavior that should be covered by tests.

def add_options_safely(params: dict, options: Optional[dict]):

api/python/quilt3/data_transfer.py:193

• The new method register_event_callback introduces new behavior that should be covered by tests.

def register_event_callback(self, event_name: str, callback: Optional[Callable]) -> None:

api/python/quilt3/data_transfer.py:209

• The new method register_write_options introduces new behavior that should be covered by tests.

def register_write_options(self, **kwargs):

[sir-sigurd]

it needs some user facing API and I guess event handlers should be stored globally instead of storing them on S3ClientProvider instance which we create almost on every call of data_transfer functions

I don't have strong opinion on that but it might make sense to allow users to provide some hook for client creation instead
it looks like we had some users modifying client creation (see #1941 (comment)) and we had attempt to allow user-provided clients (see #3765)

[drernie]

it needs some user facing API and I guess event handlers should be stored globally instead of storing them on S3ClientProvider instance which we create almost on every call of data_transfer functions

By globally do you mean as a class property? I forget the proper idiom for Python.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

api/python/quilt3/data_transfer.py:98

• [nitpick] The error message should be more descriptive. Suggestion: 'Cannot override existing key {key} in params with options: {options}. Please ensure that the options do not conflict with existing params.'

raise ValueError(f"Cannot override key `{key}` using options: {options}.")

api/python/quilt3/data_transfer.py:125

• The docstring example in register_event_options is missing a closing backtick for the code block.

</details>

[drernie]

@sir-sigurd @nl0 Okay, I think this basically works.

1. I added a manual integration test to convince myself this actually works; it is skipped for now. Should I just delete it?
2. I've added API documentation to quilt3.data_transfer.S3ClientProvider, but I'm not sure the right way to expose that

[sir-sigurd]

I added a manual integration test to convince myself this actually works; it is skipped for now. Should I just delete it?

I think you could patch something to make it create stubbed client (maybe using creating-client-class event on session?) to check that calls of some functions in data_transfer results in certain parameters in s3 client calls

I've added API documentation to quilt3.data_transfer.S3ClientProvider, but I'm not sure the right way to expose that

it doesn't look like a best way to expose it, maybe have that registration function separate from S3ClientProvider, maybe even add a separate submodule for that? 🤔

[drernie]

@sir-sigurd I think I've done all I can (can you do the gendocs?). Can you talk with @nl0 and finalize whether this is the right direction, and what else needs to be done / tested?