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

Improve Obsidian Sync for Large Vaults #1078

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

debanjum
Copy link
Member

  • Batch sync files by size to try not exceed API request payload size limits
  • Fix force sync of large vaults from Obsidian
  • Add API endpoint to delete all indexed files by file type

debanjum and others added 4 commits January 12, 2025 16:44
Previously if you tried to force sync a vault with more than 1000
files it would only end up keeping the last batch because the PUT API
call would delete all previous entries.

This change ensures the PUT call is only made for the first batch of a
force sync (regenerate) request. The rest of the API calls are PATCH,
so they do not delete files from the previous batches
@@ -325,6 +325,29 @@ def get_content_types(request: Request, client: Optional[str] = None):
return list(configured_content_types & all_content_types)


@api_content.delete("/{content_type}", status_code=200)
Copy link
Member

@sabaimran sabaimran Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API spec conflicts with out other delete endpoint, as below:

@api_content.get("/{content_source}", response_model=List[str])

I'd suggest we just use the existing deletion endpoint, and send it with type computer rather than specifying file types. Ideally, we should differentiate with Obsidian here, but that might be a bigger discussion.

We could also extend the existing API to take in content_type and content_source as query parameters, rather than path paremeters.

Copy link
Member

@sabaimran sabaimran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, tested it out locally. Just need to sort out the issue with delete APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants