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

fix propfinds with depth 0 #2918

Merged
merged 1 commit into from
Jun 8, 2022
Merged

fix propfinds with depth 0 #2918

merged 1 commit into from
Jun 8, 2022

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Jun 2, 2022

Fixed the response for propfinds with depth 0. The response now doesn't contain the shares jail anymore.

Fixes: owncloud/ocis#3704

@butonic, can you please double check that this is indeed the fix.

@C0rby C0rby requested a review from butonic June 2, 2022 10:48
@C0rby C0rby requested a review from a team as a code owner June 2, 2022 10:48
@C0rby C0rby self-assigned this Jun 2, 2022
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@butonic
Copy link
Contributor

butonic commented Jun 2, 2022

This approach omits stating the children and uses the virtual share jail as is. But the virtual share jail does not have a proper etag and mtime ...

well, /dav/spaces/{sharejailid} now need to see all mountpoints as well, which means the share jail now has to calculate etag end mtime for the root anyway. So this might actually be the fix ... but for depth 0 we still need to aggregate the mtimes, size and use the most recent etag...

@C0rby
Copy link
Contributor Author

C0rby commented Jun 2, 2022

This approach omits stating the children and uses the virtual share jail as is. But the virtual share jail does not have a proper etag and mtime ...

Do you mean stating the children of the virtual share jail?

So this might actually be the fix ... but for depth 0 we still need to aggregate the mtimes, size and use the most recent etag...

Isn't this happening here?

if mostRecentChildInfo != nil {
if rootInfo.Mtime == nil || (mostRecentChildInfo.Mtime != nil && utils.TSToUnixNano(mostRecentChildInfo.Mtime) > utils.TSToUnixNano(rootInfo.Mtime)) {
rootInfo.Mtime = mostRecentChildInfo.Mtime
if mostRecentChildInfo.Etag != "" {
rootInfo.Etag = mostRecentChildInfo.Etag
}
}
if rootInfo.Etag == "" {
rootInfo.Etag = mostRecentChildInfo.Etag
}
}
// add size of children
rootInfo.Size += aggregatedChildSize

@C0rby C0rby marked this pull request as draft June 3, 2022 12:20
@C0rby
Copy link
Contributor Author

C0rby commented Jun 7, 2022

I have now finally looked through the code again and I think that this is indeed the fix we need.

This approach omits stating the children and uses the virtual share jail as is.

We are receiving all spaces, including the mountpoints, and are aggregating their sizes which then are added to the root. So nothing is omitted AFAIU.

@C0rby C0rby marked this pull request as ready for review June 7, 2022 13:32
@sonarcloud
Copy link

sonarcloud bot commented Jun 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@C0rby C0rby marked this pull request as draft June 7, 2022 16:45
@C0rby
Copy link
Contributor Author

C0rby commented Jun 7, 2022

Oh wait, I still need to test this with subfolders. I only tested it with the root folder.

@C0rby C0rby marked this pull request as ready for review June 8, 2022 08:41
@C0rby
Copy link
Contributor Author

C0rby commented Jun 8, 2022

Oh wait, I still need to test this with subfolders. I only tested it with the root folder.

This still worked.

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.

3 participants