-
Notifications
You must be signed in to change notification settings - Fork 27
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
writefile in cs3 interface does not properly support locking #137
Comments
@DeepDiver1975 could you look into that please? |
wopi validator should catch this scenario - right? |
Good point, @wkloucek did you ever try to run the wopi validator against Reva (edge of course)? I regularly run it with the xroot interface, which does NOT suffer from this bug. |
We do run the wopi validator in scope of ocis with the wopi server. I will double check if tests.do cover this case. |
@DeepDiver1975 are you working on that? @jvillafanez can you maybe give it a try if @DeepDiver1975 can't proceed on it? |
I've tried to run the wopi validator using the https://hub.docker.com/r/deepdiver/wopi-validator-core-docker image, but it isn't working for me. Not sure if the instances need to be public
The wopiserver is running the "cs3org/wopiserver:v10.3.1" docker image. If someone has steps to reproduce, I can try to check the issue following the steps and try to work from there. |
I don't remember the exact version differences but you probably should prefer https://github.com/owncloud-ci/wopi-validator over the image you mentioned In https://github.com/owncloud/ocis/blob/c7b50b45407404bfaa62ac190165f1ae46f2319e/.drone.star#L881-L986 you can find the CI way to run the wopi validator and what we currently expect to pass. BaseWopiViewing should be on it. Maybe you can verify your setup by running a version covered by the CI, eg. https://github.com/owncloud/ocis/blob/c7b50b45407404bfaa62ac190165f1ae46f2319e/.drone.star#L944 |
This comment was marked as off-topic.
This comment was marked as off-topic.
@micbar I understand your comments belong to other issues? I'll move them there |
This is the full run for the wopi validator.
Note that I have to hack things a bit in order to work with the ".wopitest" file that the validator requires, so it isn't a clean install.
Apparently, the lock tests pass running the validator, so maybe the tests don't cover that scenario? |
Hi @jvillafanez, interesting input. It's very possible that the wopi validator does not cover this case, meaning that Microsoft does not test the case where a lock is owned by another application. The case for "us" (CERNBox), likely applicable to OCIS as well, is for files being accessed directly, not through OCIS. For info, the case is covered by the wopiserver unit test suite, though it may require a bit more tinkering to make it run: it is run by the CI only against local interface, not against a cs3 storage, which is what you'd test with the config you used for the wopi validator run. |
For the record, I can pass the full wopiserver test suite with the referenced PR, including the case that the wopivalidator does not cover. Therefore I'm going ahead and merging it. Also, following discussions in cs3org/cs3apis#226, Reva edge should be covered by external modifications, and the PR here helps raising a more meaningful error. Anyway, I understand that @jvillafanez is busy with the wopi implementation embedded in OCIS, so you're likely not going to spend further time with the cs3 wopiserver test suite. Am I right? |
As we're at last bringing our EOS driver for Reva up to speed also in terms of locking, I see we have two issues related to handling write operations in the context of locks:
lockid
is not sufficient and we need to pass also theholder
. This could be passed along as an extra opaque parameter, and I guess it would help in all implementations for logging/monitoring purposes (which apps executed which PUT operations)InitiateFileUpload
request to fail with aCODE_FAILED_PRECONDITION
error, and we should not look into any custom error message.@wkloucek for the latter point, how is Reva edge behaving? Is that case supported at all?
The text was updated successfully, but these errors were encountered: