Skip to content

refactor/introduce IContext - #9062

Draft
max-nextcloud wants to merge 50 commits into
mainfrom
refactor/introduce-icontext
Draft

refactor/introduce IContext#9062
max-nextcloud wants to merge 50 commits into
mainfrom
refactor/introduce-icontext

Conversation

@max-nextcloud

@max-nextcloud max-nextcloud commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator
  • chore(refactor): Introduce IContext and FileContext
  • chore(refactor): simplify IContext API with helper classes

Todo

Prerequisits

Backend

  • Handle sync requests with IContext.
  • get text file in AttachmentService based on context.
  • Fix unit tests for ApiService.
  • Fix 412 resonses to push requests.
  • handle saving in IContext.
  • Address refactor/file service #9028 (comment)
  • Address refactor/file service #9028 (comment)
  • check for other requests that rely on the context.
  • Migrate existing documents.
  • Add index for context_type, context_id.
  • Have a lazy context that does not setup the filesystem unless needed.
  • always create the context on the same layer.

Frontend

  • Add context arguments to createCollaborativeEditor.
  • Pass these arguments to the CollaborativeEditor component as props.
  • Use these props to connect.
  • Provide these props when saving. We still use the documentId to save and resolve the context on the backend.
  • Look into attachment handling / attachment resolver - in particular with non-file context

Testing

  • Test with public file shares
  • Check playwright tests
  • Fix playwright
    • print test since 5cca04c shows the placeholder at the bottom
  • Fix cypress tests
  • Test with collectives

Feedback

  • Separate classes
  • specify strict types on new classes
  • double check: a call to removeInactiveSessionsWithoutSteps() within the create flow was removed, and not introduced back elsewhere.
  • consider to mark the new event unstable or experimental (not aware of a phpdoc or internal established annotation. )

Documentation

  • document how to implement and register a Context

@max-nextcloud
max-nextcloud force-pushed the refactor/introduce-icontext branch 9 times, most recently from 3e17210 to a2260eb Compare August 17, 2026 10:44
@max-nextcloud max-nextcloud changed the title refactor/introduce icontext refactor/introduce IContext Aug 17, 2026
@max-nextcloud
max-nextcloud force-pushed the refactor/introduce-icontext branch 4 times, most recently from ae7bbf6 to 92e1890 Compare August 17, 2026 13:20
@max-nextcloud
max-nextcloud force-pushed the refactor/file-service branch 4 times, most recently from c9d35f7 to cd2d5b9 Compare August 18, 2026 18:29
@max-nextcloud
max-nextcloud force-pushed the refactor/introduce-icontext branch from b0a7b15 to 7cedf66 Compare August 18, 2026 18:50
@max-nextcloud
max-nextcloud force-pushed the refactor/file-service branch from cd2d5b9 to c4cca9f Compare August 19, 2026 04:32
@max-nextcloud
max-nextcloud force-pushed the refactor/introduce-icontext branch from 7cedf66 to 1bb5586 Compare August 19, 2026 04:34
@max-nextcloud
max-nextcloud force-pushed the refactor/file-service branch 2 times, most recently from 7232808 to 5907ff4 Compare August 19, 2026 05:35
@max-nextcloud
max-nextcloud force-pushed the refactor/introduce-icontext branch 3 times, most recently from 1bc577e to 58fe27f Compare August 19, 2026 09:00
Base automatically changed from refactor/file-service to main August 19, 2026 09:07
@blizzz

blizzz commented Aug 19, 2026

Copy link
Copy Markdown
Member

So far I read up to commit chore(tweak): return first editable file early. I love your iterative style and the discipline you keep with having small, digestible and sensible commits! My impression is also that with having the abstraction layers, there are many places where it just gets cleaner, and having specific logic better capsuled!

What I stumble over is that the IContext file contains the additional classes DocumentData, SessionInfo, NewSessionData. Usually this is something we keep separate and I am not sure it will always works with autoloading? Also about fellow developer expectations. Maybe something to consider.

Maybe this is also OK, just to double check: a call to removeInactiveSessionsWithoutSteps() within the create flow was removed, and not introduced back elsewhere. Alright if intentional of course.

No biggie: new classes miss the declare(strict_types=1); statement.

Can consider to mark the new event unstable or experimental, though I am not aware of a phpdoc or internal established annotation.

@blizzz

blizzz commented Aug 19, 2026

Copy link
Copy Markdown
Member

On my dev instance I get an error during Migration:

{
  "reqId": "miQrQfvExjkhgL8Z3V6T",
  "level": 3,
  "time": "2026-08-19T14:14:45+00:00",
  "remoteAddr": "",
  "user": "--",
  "app": "updater",
  "method": "",
  "url": "--",
  "scriptName": "occ",
  "message": "\\OC\\Updater::failure: Exception: Database error when running migration 090000Date20260817110024 for app text\nAn exception occurred while executing a query: SQLSTATE[23502]: Not null violation: 7 ERROR:  column \"context_type\" of relation \"oc_text_documents\" contains null values",
  "userAgent": "--",
  "version": "35.0.0.3",
  "occ_command": [
    "occ",
    "upgrade"
  ],
  "data": {
    "app": "updater"
  }
}

DB is postgres.

Probably happens because the column is added as NOT NULL but the rows would be empty (=> NULL). Maybe set the default to "file"?

UPDATE: same with context_id. Default to 0?

When creating a new session we load the document by context.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Droped in a7a63d2 .

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
File ids used to be document ids. But not anymore.

In all current calls to `resetDocument` the parameter send actually is a file id.

Follow up: Make this generic for all context types.--signoff

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
* Use IShare and IUser in the file context api explicitely.
* Make creating the context cheap.
* File is accessed when needed and then cached.

Signed-off-by: Max <max@nextcloud.com>
The two used to be the same - but now we have contexts to store the file id.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
For direct editing pass the user to the attachment service.

For the sake of consistency pass IShare|IUser from the controllers everywhere.
This way we can also build on the permission checks and getters in the controllers.

Signed-off-by: Max <max@nextcloud.com>
Use exceptions to destinguish between not found and forbidden.

We try to not expose wether a file exists or not.
But if the file exists but download permissions have not been granted
we want to be explicit about that and return a 403 response code.

Signed-off-by: Max <max@nextcloud.com>
The directory name cannot be derived from the document id anymore
as the document id and the text file id now differ.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
We can detect the conflict afterwards and handle it in the document service.

This makes the `updateDocument` function easier to understand and reuse.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
max-nextcloud force-pushed the refactor/introduce-icontext branch from 173ad94 to fe36add Compare September 2, 2026 13:45
If this event is emitted for a context
the related document and all connected sessions will be cleared.

This will cause the next requests to be answered with 412 responses
which in turn cause the client to render a conflict screen.

Signed-off-by: Max <max@nextcloud.com>
* Separates the file specific concerns from the generic document concerns.
* This makes `DocumentService` independent of `LockService` and `FileService`.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
max-nextcloud force-pushed the refactor/introduce-icontext branch from fe36add to 4362657 Compare September 2, 2026 15:16
#onSync
#onOpened
#processingVersion = 0
#documentId = 0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

return new DataResponse('', Http::STATUS_NOT_FOUND);
}

private function getAuth(string $shareToken, bool $updatePermissionRequired = true): IShare|IUser {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

wonder whether it makes sense to have an umbrella interface here to use as return type hint. Or a wrapper class. Probably overkill. Benefit would be not to have a union return, but at some point you've got to figure out when to work with a share or user at the callee, so… just thinking loudly.

@@ -361,6 +361,10 @@ public function autosave(Document $document, IContext $context, int $version, st
if ($lastMTime > 0 && !$force && !$this->cache->get('document-save-lock-' . $document->id)) {
$updatedDocument = $context->updateDocument($document);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

$document and $updatedDocument are they same. Objects are passed by reference, and FileContext does not clone $document but just returns it (provided the checks pass and don't return null)

Comment on lines +56 to +57
$oldChecksum = $document->getChecksum();
$newChecksum = $updatedDocument->getChecksum();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

here as well, same document instance that is acted upon

/**
* @template-implements IEventListener<Event|DocumentContentUpdated>
*/
class DocumentContentUpdatedListener implements IEventListener {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It needs to be registered yet

Comment thread src/apis/connect.ts
: generateUrl('/apps/text')
const url = `${_baseUrl}/session/${params.fileId}/create`
export async function openShare(params: OpenShareParams): Promise<{ connection: Connection, data: OpenData }> {
const url = generateUrl('/apps/text/public/session/123/create')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

debug leftover?

$userFolder = $this->rootFolder->getUserFolder($userId);
$file = $userFolder->getFirstNodeById($documentId);
private function getTextFile(int $documentId, IShare|IUser $auth): File {
$document = $this->documentMapper->find($documentId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could also result in a DoesNotExistException, which is not handled

}

public function getContext(string $type, int $id, IShare|IUser $auth): IContext {
$factoryClassName = $this->getContexts()[$type];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
$factoryClassName = $this->getContexts()[$type];
$factoryClassName = $this->getContexts()[$type] ?? null;

to avoid log spam on non-existing keys


export interface AutofocusOptions {
fileId: number | null
id: number | null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is now context type + id, innit?

shareToken = undefined,
} = defineProps<{
fileId?: number
defineProps<{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I get the hint:

ViewerComponent.vue migrated to type-only defineProps without withDefaults, making active and isEmbedded required and dropping the onLoadedHandler: () => {} default; Viewer mounts this externally via ViewerView.js (props: ViewerComponent.props), so expect missing-required-prop warnings and @loaded="undefined". And createEditor.ts still gates on Boolean(options.fileId), so a caller using the newly-advertised context option falls through to createMarkdownContentEditor.

But I do not fully understand it 🙈

Comment thread lib/Db/DocumentMapper.php
Comment on lines +49 to +50
->where($qb->expr()->eq('context_type', $qb->createNamedParameter($type)))
->andWhere($qb->expr()->eq('context_id', $qb->createNamedParameter($id)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The optional query type is not specified, at least for context_id it should be IQueryBuilder::PARAM_INT. IQueryBuilder::PARAM_STR is the default.

}

$column = $table->getColumn('id');
if (!$column->getAutoincrement()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually, I am not sure whether this is safe, also across DB types. I do not see similar code in Nextcloud. Maybe double check with @nickvergessen

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding autoincrement does not work as far as I recall. Had it in another app when we had to add the primary keys to all databases.

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