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

Collaboration connector refactor #9771

Merged
merged 6 commits into from
Aug 13, 2024
Merged

Conversation

jvillafanez
Copy link
Member

Description

Refactor connector to homogenize the responses. This means that almost all the connector methods will return a ConnectorResponse which will include the HTTP status, headers and body. Note that the body is expected to be JSON-encoded before sending it to the client.
There are new NewResponse* methods to simplify the creation of common responses. These include a response with just the HTTP status and a response with status and lock id. For more complex responses, you'll need to create the response yourself.

Code of the httpAdapter is greatly simplified with these changes, the code of the connector is mostly homogenized (only some exceptions), and the behavior is barely touched (only in the PutRelativeFile)

In addition, mockery has been updated to 2.43.2 because mocks were already created with that version.

Exceptions:

  • https://github.com/owncloud/ocis/compare/collaboration_connector_refactor?expand=1#diff-57a0aa20d4a5545fa33573bfb8120aea99fc483bc370054528f0e8e0629728f1R729-R739 The response is built manually because we'll need a lot of parameters and a custom NewResponse* method to simplify the call.
  • The GetLock method returns a lock id with a 200 status. This is why we opted to supply a status in the NewResponseWithLock method even though the rest of the cases the lock would be sent with a 409 status.
  • Some of the code for the PutRelativeFile methods has been adjusted because the underlying PutFile call also returns a ConnectorResponse. The behavior of both methods should still be the same.
  • The GetFile method remains unchanged with the refactor. The main problem is that we'd need a switch of responsibilities: the connector method would need to return a reader for the httpAdapter to copy the contents, however this will cause problems with the "bodycloser" linter because the connector would need to send a request but can't close the body of the response.

Related Issue

No open issue

Motivation and Context

With the only exception of the GetFile method in the httpAdapter, the rest of the methods are straightforward.
Having the same response for most of the connector methods (with the exception of the GetFile) is easier to understand the code.

How Has This Been Tested?

unit tests and wopi validator

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@jvillafanez jvillafanez self-assigned this Aug 9, 2024
Copy link

update-docs bot commented Aug 9, 2024

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@jvillafanez
Copy link
Member Author

I don't think we need a changelog entry for this, so good to merge?

@kobergj
Copy link
Collaborator

kobergj commented Aug 13, 2024

No please add a changelog. Missed it during review 😢

Copy link

sonarcloud bot commented Aug 13, 2024

@jvillafanez jvillafanez merged commit bb78b16 into master Aug 13, 2024
4 checks passed
@jvillafanez jvillafanez deleted the collaboration_connector_refactor branch August 13, 2024 09:38
ownclouders pushed a commit that referenced this pull request Aug 13, 2024
@ScharfViktor ScharfViktor mentioned this pull request Aug 20, 2024
21 tasks
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