-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: Handle Virtual Documents persistance in PouchLink #1486
feat: Handle Virtual Documents persistance in PouchLink #1486
Conversation
bdea993
to
2f00064
Compare
4c4c489
to
3d29bb0
Compare
9a3b23b
to
dddcdbc
Compare
3d29bb0
to
b425f41
Compare
dddcdbc
to
17d7454
Compare
b425f41
to
bcb49e6
Compare
17d7454
to
c376d72
Compare
bcb49e6
to
02632b9
Compare
c376d72
to
255e57e
Compare
e842a07
to
a761b3b
Compare
a761b3b
to
b8abde2
Compare
255e57e
to
0037e6f
Compare
b8abde2
to
d5677b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'aime pas trop le terme de persistData() car c'est pas de la persistance générique qu'on met là-dedans vu qu'on vient rajouter du cozyLocalOnly, qu'on dégage le _type etc. C'est pas juste on file de la data et on la persiste.
Mais à part ça, top !
* @returns {Promise<void>} | ||
*/ | ||
async persistVirtualDocument(document) { | ||
if (document && !document.meta?.rev && !document.cozyLocalOnly) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_rev
and not only meta.rev
, which is only for json-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in this commit: 62290ed
However I did not do actual tests yet, I expect to do them tomorrow
if (!Array.isArray(data)) { | ||
await this.persistVirtualDocument(data) | ||
} else { | ||
for (const document of data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this forces a full iteration on all docs for all requests, which is kind of a pity, especially since we actually need this in quite few cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I don't think this will cost that much as this is a pure JS loop that check objects properties. Also I don't have better solution, so I propose to keep it that way for now but to be attentive about performances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but to be attentive about performances
I'm pretty sure we won't detect this overhead. I guess there would be some way to improve, but let's keep it simple for now
@@ -384,6 +386,44 @@ class PouchLink extends CozyLink { | |||
return this.executeQuery(operation) | |||
} | |||
} | |||
|
|||
async persistData(data, forward = doNothing) { | |||
const docWithoutType = sanitized(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sanitized
sounds very generic for a function that simply removes the _type
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #1524
const docWithoutType = sanitized(data) | ||
docWithoutType.cozyLocalOnly = true | ||
|
||
const oldDoc = await this.getExistingDocument(data._id, data._type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a detail, but in this very specific case, I wonder if we should not check on id
and type
instead, as it is what is returned in the json-api format (we add _id
and _type
in the normalization step)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should do some verifications first before answering that. I'll keep you in touch
} | ||
|
||
const db = this.pouches.getPouch(data._type) | ||
await db.put(docWithoutType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the attributes
object that is returned by the stack for json-api docs? I assume we should handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "handle it"? We should remove it? Or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that a document returned by the stack will look like this with json-api formatting:
{
id: 123,
type: 'io.cozy.todo',
attributes: {
name: 'do stuff'
}
}
Thanks to the normalization, the attributes
is handled by spreading the content at the document root, typically here: https://github.com/cozy/cozy-client/blob/master/packages/cozy-stack-client/src/SettingsCollection.js#L17
But it still exists, so I think we should remove it before saving it to pouchdb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #1525
document && | ||
!document.meta?.rev && | ||
!document.cozyLocalOnly && | ||
!document.cozyFromPouch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this whole commit (5762d43) is not necessary anymore if we correctly handle the ._rev
condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be correct, but I should do some verifications first to confirm the theory. I'll keep you in touch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #1524
Est-ce qu'un |
d5677b5
to
95978d3
Compare
When specifying fields in a query, e.g. `Q('io.cozy.todos').where({done: true}).select(['date'])`, the revision was missing if not explicitly given. This is now problematic because we rely on the revision existence to identify "virtual" documents, i.e. not persisted in CouchDB, that never have any revision. See #1486 for more insights.
This commit is a copy of #1517 applied to CozyPouchLink When specifying fields in a query, e.g. `Q('io.cozy.todos').where({done: true}).select(['date'])`, the revision was missing if not explicitly given. This is now problematic because we rely on the revision existence to identify "virtual" documents, i.e. not persisted in CouchDB, that never have any revision. See #1486 for more insights.
This guard has been added to handle documents with no `meta.rev` but since we added a check on `_rev` (in addition to `meta.rev`) to trigger the persistence, then we don't need this guard anymore This replies to #1486 (comment) Related commit: b797eb3 Related commit: 62290ed
Those doc's attributes are specific to the JSON API and should not be inserted into the Pouch database This implies that we will have a difference on documents regarding if they are served through the cozy-stack or through a local PouchDB, the first one may include those fields in their result, but not the second one So from now we should avoid, as much as possible, to relies on the `attributes` member to prevent bugs on Offline mode. Multiple commits to fix usages of `attributes` in cozy-client will be done after this one Usage of `attributes` and `meta` members on cozy-app will also have to be fixed. This will be a requirement to implement Offline mode on cozy-apps This replies to #1486 (comment)
Those doc's attributes are specific to the JSON API and should not be inserted into the Pouch database This implies that we will have a difference on documents regarding if they are served through the cozy-stack or through a local PouchDB, the first one may include those fields in their result, but not the second one So from now we should avoid, as much as possible, to relies on the `attributes` member to prevent bugs on Offline mode. Multiple commits to fix usages of `attributes` in cozy-client will be done after this one Usage of `attributes` and `meta` members on cozy-app will also have to be fixed. This will be a requirement to implement Offline mode on cozy-apps This replies to #1486 (comment)
In previous implementation we did not await for the `persistVirtualDocuments()` result before completing the request This was done because the persistance was not mandatory for the request's result and doing it in parallel would be a good way to optimize timings But in some recent experiments we found a scenario where we would benefit from the `await` in order to some external code until the data is actually persisted Related PR: #1486
Those doc's attributes are specific to the JSON API and should not be inserted into the Pouch database This implies that we will have a difference on documents regarding if they are served through the cozy-stack or through a local PouchDB, the first one may include those fields in their result, but not the second one So from now we should avoid, as much as possible, to relies on the `attributes` member to prevent bugs on Offline mode. Multiple commits to fix usages of `attributes` in cozy-client will be done after this one Usage of `attributes` and `meta` members on cozy-app will also have to be fixed. This will be a requirement to implement Offline mode on cozy-apps This replies to #1486 (comment)
In previous implementation we did not await for the `persistVirtualDocuments()` result before completing the request This was done because the persistance was not mandatory for the request's result and doing it in parallel would be a good way to optimize timings But in some recent experiments we found a scenario where we would benefit from the `await` in order to some external code until the data is actually persisted Related PR: #1486
Those doc's attributes are specific to the JSON API and should not be inserted into the Pouch database This implies that we will have a difference on documents regarding if they are served through the cozy-stack or through a local PouchDB, the first one may include those fields in their result, but not the second one So from now we should avoid, as much as possible, to relies on the `attributes` member to prevent bugs on Offline mode. Multiple commits to fix usages of `attributes` in cozy-client will be done after this one Usage of `attributes` and `meta` members on cozy-app will also have to be fixed. This will be a requirement to implement Offline mode on cozy-apps This replies to #1486 (comment)
In previous implementation we did not await for the `persistVirtualDocuments()` result before completing the request This was done because the persistance was not mandatory for the request's result and doing it in parallel would be a good way to optimize timings But in some recent experiments we found a scenario where we would benefit from the `await` in order to some external code until the data is actually persisted Related PR: #1486
In previous implementation we did not await for the `persistVirtualDocuments()` result before completing the request This was done because the persistance was not mandatory for the request's result and doing it in parallel would be a good way to optimize timings But in some recent experiments we found a scenario where we would benefit from the `await` in order to some external code until the data is actually persisted Related PR: #1486
This replies to #1486 (comment)
This commit is a copy of #1517 applied to CozyPouchLink When specifying fields in a query, e.g. `Q('io.cozy.todos').where({done: true}).select(['date'])`, the revision was missing if not explicitly given. This is now problematic because we rely on the revision existence to identify "virtual" documents, i.e. not persisted in CouchDB, that never have any revision. See #1486 for more insights.
This guard has been added to handle documents with no `meta.rev` but since we added a check on `_rev` (in addition to `meta.rev`) to trigger the persistence, then we don't need this guard anymore This replies to #1486 (comment) Related commit: b797eb3 Related commit: 62290ed
Those doc's attributes are specific to the JSON API and should not be inserted into the Pouch database This implies that we will have a difference on documents regarding if they are served through the cozy-stack or through a local PouchDB, the first one may include those fields in their result, but not the second one So from now we should avoid, as much as possible, to relies on the `attributes` member to prevent bugs on Offline mode. Multiple commits to fix usages of `attributes` in cozy-client will be done after this one Usage of `attributes` and `meta` members on cozy-app will also have to be fixed. This will be a requirement to implement Offline mode on cozy-apps This replies to #1486 (comment)
In previous implementation we did not await for the `persistVirtualDocuments()` result before completing the request This was done because the persistance was not mandatory for the request's result and doing it in parallel would be a good way to optimize timings But in some recent experiments we found a scenario where we would benefit from the `await` in order to some external code until the data is actually persisted Related PR: #1486
This replies to #1486 (comment)
This commit is a copy of #1517 applied to CozyPouchLink When specifying fields in a query, e.g. `Q('io.cozy.todos').where({done: true}).select(['date'])`, the revision was missing if not explicitly given. This is now problematic because we rely on the revision existence to identify "virtual" documents, i.e. not persisted in CouchDB, that never have any revision. See #1486 for more insights.
This guard has been added to handle documents with no `meta.rev` but since we added a check on `_rev` (in addition to `meta.rev`) to trigger the persistence, then we don't need this guard anymore This replies to #1486 (comment) Related commit: b797eb3 Related commit: 62290ed
Those doc's attributes are specific to the JSON API and should not be inserted into the Pouch database This implies that we will have a difference on documents regarding if they are served through the cozy-stack or through a local PouchDB, the first one may include those fields in their result, but not the second one So from now we should avoid, as much as possible, to relies on the `attributes` member to prevent bugs on Offline mode. Multiple commits to fix usages of `attributes` in cozy-client will be done after this one Usage of `attributes` and `meta` members on cozy-app will also have to be fixed. This will be a requirement to implement Offline mode on cozy-apps This replies to #1486 (comment)
In previous implementation we did not await for the `persistVirtualDocuments()` result before completing the request This was done because the persistance was not mandatory for the request's result and doing it in parallel would be a good way to optimize timings But in some recent experiments we found a scenario where we would benefit from the `await` in order to some external code until the data is actually persisted Related PR: #1486
This replies to #1486 (comment)
This commit is a copy of #1517 applied to CozyPouchLink When specifying fields in a query, e.g. `Q('io.cozy.todos').where({done: true}).select(['date'])`, the revision was missing if not explicitly given. This is now problematic because we rely on the revision existence to identify "virtual" documents, i.e. not persisted in CouchDB, that never have any revision. See #1486 for more insights.
This guard has been added to handle documents with no `meta.rev` but since we added a check on `_rev` (in addition to `meta.rev`) to trigger the persistence, then we don't need this guard anymore This replies to #1486 (comment) Related commit: b797eb3 Related commit: 62290ed
Those doc's attributes are specific to the JSON API and should not be inserted into the Pouch database This implies that we will have a difference on documents regarding if they are served through the cozy-stack or through a local PouchDB, the first one may include those fields in their result, but not the second one So from now we should avoid, as much as possible, to relies on the `attributes` member to prevent bugs on Offline mode. Multiple commits to fix usages of `attributes` in cozy-client will be done after this one Usage of `attributes` and `meta` members on cozy-app will also have to be fixed. This will be a requirement to implement Offline mode on cozy-apps This replies to #1486 (comment)
In previous implementation we did not await for the `persistVirtualDocuments()` result before completing the request This was done because the persistance was not mandatory for the request's result and doing it in parallel would be a good way to optimize timings But in some recent experiments we found a scenario where we would benefit from the `await` in order to some external code until the data is actually persisted Related PR: #1486
This replies to #1486 (comment)
This commit is a copy of #1517 applied to CozyPouchLink When specifying fields in a query, e.g. `Q('io.cozy.todos').where({done: true}).select(['date'])`, the revision was missing if not explicitly given. This is now problematic because we rely on the revision existence to identify "virtual" documents, i.e. not persisted in CouchDB, that never have any revision. See #1486 for more insights.
This guard has been added to handle documents with no `meta.rev` but since we added a check on `_rev` (in addition to `meta.rev`) to trigger the persistence, then we don't need this guard anymore This replies to #1486 (comment) Related commit: b797eb3 Related commit: 62290ed
Those doc's attributes are specific to the JSON API and should not be inserted into the Pouch database This implies that we will have a difference on documents regarding if they are served through the cozy-stack or through a local PouchDB, the first one may include those fields in their result, but not the second one So from now we should avoid, as much as possible, to relies on the `attributes` member to prevent bugs on Offline mode. Multiple commits to fix usages of `attributes` in cozy-client will be done after this one Usage of `attributes` and `meta` members on cozy-app will also have to be fixed. This will be a requirement to implement Offline mode on cozy-apps This replies to #1486 (comment)
In previous implementation we did not await for the `persistVirtualDocuments()` result before completing the request This was done because the persistance was not mandatory for the request's result and doing it in parallel would be a good way to optimize timings But in some recent experiments we found a scenario where we would benefit from the `await` in order to some external code until the data is actually persisted Related PR: #1486
Note
This PR follows #1483
When a PouchLink is configured, the synchronization process would retrieve only documents that exists on the remote database
Those documents would be served as usual when a
.query()
is made using the PouchLinkUnfortunately, not all
.query()
correspond to a document that exists on the remote database. With the collection mechanism, some documents are retrieved using an alternative endpoint and then the cozy-stack generate the document on-demandFor example this is the case for
io.cozy.settings.flags
,io.cozy.settings.context
andio.cozy.settings.disk-usage
Because those documents does not actually exist on the remote database, we cannot sync them using the PouchDB sync mechanism and so they won't be available when the device is offline
To fix this, we chose to manually insert those document in the local PouchDB when we detect that a
.query()
call retrieve them from the cozy-stackThe easiest way we found to detect them is to check for the existence of
meta.rev
object in the.query()
result. If norev
is present, this means that the documents does not come from the remote database but has been dynamically generated by the cozy-stackThose documents are inserted into existing PouchBD with the
cozyLocalOnly
attributes, so we know that they should not be replicated to the remote database on next syncIn order to make the persist mechanism possible, we implemented a new
persistData()
method to the CozyLink interfaceThe impact is that every
Link
should now implement thepersistData()
interface that should persist the data if it is able to do it, or to forward the action otherwiseIf no
Link
process the action, then the final "default" method will do nothing (contrary to the default forrequest()
that would throw an error)BREAKING CHANGE: CozyClient's links should now implement a
persistData()
method that should persist the given data if it is able to do it, or to forward the action to the nextLink
otherwiseRelated PRs: