Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate core model logic from top-level asset service layer functions #1991

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

jjnesbitt
Copy link
Member

@jjnesbitt jjnesbitt commented Aug 4, 2024

Depends on #1886

As a part of the audit backend PR, I noticed that we do something peculiar in the change_asset service layer function, which required an undesirable workaround for audit. The change_asset function at its core just calls remove_asset_from_version and add_asset_to_version sequentially. This achieves the desired outcome, but with the following downsides:

  1. Each service layer function (including change_asset) contains its own set of checks and filters to ensure the asset being created/modified/destroyed is not malformed. This is largely similar between the services, but they each differ slightly. So when remove_asset_from_version and add_asset_to_version are called from change_asset, they are each making checks that were already made in change_asset, making the overall operation inefficient.
  2. This general architecture of calling one service function from another is not ideal, as it means that those two top level service layer functions have the responsibility of both their intended service layer purpose, as well as needing to conform to being called by other service layer functions, which could potentially diverge in use case.
  3. The core model logic is intertwined with the service layer functions, making it impossible to elegantly perform the low level model operations without the service layer side effects. This lead to the do_audit flag being introduced into add_asset_to_version and remove_asset_from_version, to workaround this limitation.

This PR separates out the core model logic from those two respective functions, allowing all of them, including change_asset, to utilize them. This removes the need for the do_audit flag, and generally cleans up the asset service layer a bit.

Once #1886 is merged, this can be rebased off master and fully evaluated.

@jjnesbitt jjnesbitt force-pushed the refactor-asset-service-layer branch 2 times, most recently from c2b7769 to c30d2eb Compare August 4, 2024 17:11
Base automatically changed from audit-backend to master August 13, 2024 14:35
@jjnesbitt jjnesbitt marked this pull request as ready for review August 14, 2024 19:52
dandiapi/api/services/asset/__init__.py Show resolved Hide resolved
dandiapi/api/services/asset/__init__.py Outdated Show resolved Hide resolved
dandiapi/api/services/asset/__init__.py Outdated Show resolved Hide resolved
@jjnesbitt
Copy link
Member Author

It seems the current linting errors are from a rule change in ruff, affecting our pytest marks. I'd like to defer that fix to a separate PR.

@jjnesbitt jjnesbitt merged commit d373b91 into master Aug 16, 2024
10 of 11 checks passed
@jjnesbitt jjnesbitt deleted the refactor-asset-service-layer branch August 16, 2024 18:13
@dandibot
Copy link
Member

🚀 PR was released in v0.3.94 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants