Skip to content

Conversation

@butonic
Copy link
Contributor

@butonic butonic commented Dec 2, 2025

This PR moves the ocdav handler back into the frontend. This removes one usage of the go micro service.

@butonic butonic requested a review from rhafer December 2, 2025 15:00
@butonic butonic self-assigned this Dec 2, 2025
@butonic butonic added Type:Maintenance E.g. technical debt, packaging, etc. Type:Breaking-Change labels Dec 2, 2025
@github-project-automation github-project-automation bot moved this to Qualification in OpenCloud Team Board Dec 2, 2025
@butonic butonic force-pushed the ocdav-to-frontent branch 6 times, most recently from a9f558d to 0947bf7 Compare December 5, 2025 09:39
@butonic
Copy link
Contributor Author

butonic commented Dec 5, 2025

it seems reva handles a leading // differently:

  Scenario: send PROPFIND requests to webDav endpoints with 2 slashes using the spaces WebDAV API                                 # /woodpecker/src/github.com/opencloud-eu/opencloud/tests/acceptance/features/coreApiAuth/webDavSpecialURLs.feature:155
    When user "Alice" requests these endpoints with "PROPFIND" to get property "d:href" about user "Alice"                        # AuthContext::theUserRequestsTheseEndpointsToGetOrSetPropertyAboutUser()
      | endpoint                                 |
      | //dav//spaces/%spaceid%/textfile1.txt    |
      | /dav//spaces/%spaceid%/PARENT/parent.txt |
      | /dav//spaces/%spaceid%/PARENT            |
      | //dav/spaces//%spaceid%//FOLDER          |
    Then the HTTP status code of responses on each endpoint should be "200,207,207,200" on OpenCloud or "207,207,207,207" on reva # FeatureContext::theHTTPStatusCodeOfResponsesOnEachEndpointShouldBeOcReva()
      Expected HTTP status codes: "200,207,207,200". Found HTTP status codes: "405,207,207,405"
      Failed asserting that false is true.

  Scenario: send PROPPATCH requests to webDav endpoints with 2 slashes                                                 # /woodpecker/src/github.com/opencloud-eu/opencloud/tests/acceptance/features/coreApiAuth/webDavSpecialURLs.feature:165
    When user "Alice" requests these endpoints with "PROPPATCH" to set property "d:getlastmodified" about user "Alice" # AuthContext::theUserRequestsTheseEndpointsToGetOrSetPropertyAboutUser()
      | endpoint                                 |
      | //webdav/textfile0.txt                   |
      | //dav//files/%username%/textfile1.txt    |
      | /dav//files/%username%/PARENT/parent.txt |
      | /webdav//PARENT                          |
      | //dav//files/%username%//FOLDER          |
    Then the HTTP status code of responses on each endpoint should be "200,200,400,400,200" respectively               # FeatureContext::theHTTPStatusCodeOfResponsesOnEachEndpointShouldBe()
      Expected HTTP status codes: "200,200,400,400,200". Found HTTP status codes: "405,405,400,400,405"
      Failed asserting that false is true.

@butonic
Copy link
Contributor Author

butonic commented Dec 5, 2025

@ScharfViktor the tests were expecting the wrong status codes. I corrected them and CI should now become green.

@butonic butonic moved this from Qualification to In Progress in OpenCloud Team Board Dec 5, 2025
@butonic butonic requested a review from aduffeck December 8, 2025 09:48
}

type OCDav struct {
Prefix string `yaml:"prefix" env:"OCDAV_HTTP_PREFIX" desc:"A URL path prefix for the handler." introductionVersion:"1.0.0"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we introduce FRONTEND_OCDAV_* variables and deprecate the OCDAV_* ones for consistency?

Copy link
Contributor Author

@butonic butonic Dec 8, 2025

Choose a reason for hiding this comment

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

I would rather aim for dropping the service prefix. @rhafer proposed that and since we are deploying only one process in the compose stack and two or three in the helm chart, I agree. In this case OCDAV_ would match the handler, so I think reusing them is fine.

IIRC @dragonchaser is trying to reduce the number of env vars as well, so he might have an opinion as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be confusing when this is a service specific variable and there is no standalone ocdav-service anymore.
IMHO @aduffeck change would make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, keep the old env var OCDAV_HTTP_PREFIX but allow overriding it with a more specific one?

Suggested change
Prefix string `yaml:"prefix" env:"OCDAV_HTTP_PREFIX" desc:"A URL path prefix for the handler." introductionVersion:"1.0.0"`
Prefix string `yaml:"prefix" env:"OCDAV_HTTP_PREFIX;FRONTEND_OCDAV_HTTP_PREFIX" desc:"A URL path prefix for the handler." introductionVersion:"1.0.0"`

For now, I'm fine with this as it would follow the pattern that we currently have. I wonder if we can then drop the FRONTEND_OCDAV_HTTP_PREFIX later ...

@butonic
Copy link
Contributor Author

butonic commented Jan 7, 2026

cc @ScharfViktor @saw-jan flaky run

runsh: Total unexpected failed scenarios throughout the test run:
apiCollaboration/wopi.feature:495

flaky run

runsh: Total unexpected failed scenarios throughout the test run:
apiSearchContent/extractedProps.feature:426

@saw-jan
Copy link
Contributor

saw-jan commented Jan 7, 2026

cc @ScharfViktor @saw-jan flaky run

runsh: Total unexpected failed scenarios throughout the test run:
apiCollaboration/wopi.feature:495

flaky run

runsh: Total unexpected failed scenarios throughout the test run:
apiSearchContent/extractedProps.feature:426

wopi API tests seem to be more flaky. I have created an issue for this #2110

@butonic
Copy link
Contributor Author

butonic commented Jan 7, 2026

cc @ScharfViktor @saw-jan flaky run

runsh: Total unexpected failed scenarios throughout the test run:
coreApiWebdavMove2/moveShareOnOpencloud.feature:283

@ScharfViktor
Copy link
Contributor

cc @ScharfViktor @saw-jan flaky run

but how??

When user "Alice" moves folder "Shares/testshare/testfile.txt" to "Shares/testshare/child/testfile.txt" using the WebDAV API
Then the HTTP status code should be "201"
And as "Alice" file "/Shares/testshare/child/testfile.txt" should exist -> ✅️ file is moved
And as "Brian" file "/testshare/child/testfile.txt" should exist
And as "Alice" file "/Shares/testshare/testfile.txt" should not exist -> It turns out that we have the file both here and there.

I see that file is exist but we moved it in line

### And as "Alice" file "/Shares/testshare/testfile.txt" should not exist
		_______________________________________________________________________

		==> REQUEST
		PROPFIND /remote.php/dav/spaces/a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668/testshare/testfile.txt
		X-Request-ID: coreApiWebdavMove2/moveShareOnOpencloud.feature:283-275
		==> REQ BODY
		<?xml version="1.0"?>
				<d:propfind
				   xmlns:d="DAV:"
				   xmlns:oc="[http://owncloud.org/ns"](http://owncloud.org/ns%22)
				   xmlns:ocs="[http://open-collaboration-services.org/ns"](http://open-collaboration-services.org/ns%22)
				   >
				    <d:prop><d:getetag/><d:resourcetype/></d:prop>
				</d:propfind>

		<== RESPONSE
		207 Multi-Status
		X-Xss-Protection: 1; mode=block
		<== RES BODY
		<d:multistatus xmlns:s="[http://sabredav.org/ns"](http://sabredav.org/ns%22) xmlns:d="DAV:" xmlns:oc="[http://owncloud.org/ns"><d:response><d:href>/remote.php/dav/spaces/a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668/testshare/testfile.txt</d:href><d:propstat><d:prop><d:getetag>"d86b0cd3c4b5d8e5b8c0eb91a7a24bf4"</d:getetag><d:resourcetype></d:resourcetype></d:prop><d:status>HTTP/1.1](http://owncloud.org/ns%22%3E%3Cd:response%3E%3Cd:href%3E/remote.php/dav/spaces/a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668/testshare/testfile.txt%3C/d:href%3E%3Cd:propstat%3E%3Cd:prop%3E%3Cd:getetag%3E%22d86b0cd3c4b5d8e5b8c0eb91a7a24bf4%22%3C/d:getetag%3E%3Cd:resourcetype%3E%3C/d:resourcetype%3E%3C/d:prop%3E%3Cd:status%3EHTTP/1.1) 200 OK</d:status></d:propstat></d:response></d:multistatus>

@butonic
Copy link
Contributor Author

butonic commented Jan 7, 2026

cc @ScharfViktor @saw-jan flaky run

runsh: Total unexpected failed scenarios throughout the test run:
apiSharingNgLinkSharePermission/createLinkShare.feature:806

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic force-pushed the ocdav-to-frontent branch from 218a050 to 36f9d38 Compare January 7, 2026 14:04
@butonic
Copy link
Contributor Author

butonic commented Jan 7, 2026

I rebased on master to get #2099

@butonic butonic merged commit 4c600d8 into main Jan 7, 2026
60 checks passed
@butonic butonic deleted the ocdav-to-frontent branch January 7, 2026 16:17
@github-project-automation github-project-automation bot moved this from In Progress to Done in OpenCloud Team Board Jan 7, 2026
@openclouders openclouders mentioned this pull request Jan 7, 2026
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type:Breaking-Change Type:Maintenance E.g. technical debt, packaging, etc.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants