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

New ViewService + Fix bug with memory monitoring #428

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

lbwexler
Copy link
Member

No description provided.

@lbwexler lbwexler requested a review from ghsolomon December 19, 2024 18:05
@@ -176,6 +176,11 @@ class XhController extends BaseController {
renderJSON(ret.formatForClient())
}

def findJsonBlob(String type, String name) {
def ret = jsonBlobService.find(type, name)
renderJSON(ret?.formatForClient())
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious if it would be appropriate to return a 404 if we can't find the requested blob

Copy link
Contributor

Choose a reason for hiding this comment

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

Also realized this only returns a blob if it is owned by the user, whereas the listJsonBlobs endpoints returns blobs for which the user passesAcl. I see why you did this, but I wonder if it could be confusing-- i.e. "Why do I see a blob in the list of all blobs, but when I try to look it up directly using the findJsonBlob endpoint, nothing is returned?"

}
return blob
@Transactional
JsonBlob createOrUpdate(String type, String name, Map data, String username = username) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work for blobs that are not owned by the authenticated user? In the context of views, blobs describing "Global" views with null owners?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here I was definitely mirrorring the create semantics, which only allow you to create a blob that you own. Will comment that better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I just misunderstood it - looks good to me :)

@lbwexler lbwexler changed the title Checkpoint Update JsonBlob API Misc + Fix bug with memory management. Dec 22, 2024
@lbwexler lbwexler changed the title Update JsonBlob API Misc + Fix bug with memory management. New ViewService + Fix bug with memory monitoring Dec 24, 2024
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