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

Fix inversePatches applying order #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shqld
Copy link

@shqld shqld commented Sep 25, 2024

(本家でも再現するので近いうちにissueを送る予定)

以下のように、「オブジェクト内部に変更 → (手前の要素を削除するなどして)そのオブジェクトのインデックスを変更」した場合、

draft[3].id *= 10;
draft.splice(0, 1);

以下のようにinversePatchesが存在しない要素を参照(replace)してしまいapply時にエラーとなる。

prevState: [0, 1, 2, 3]
nextState: [1, 2, 30]

patches:
1. replace arr[3]
2. remove arr[0]

inversePatches:
1. replace arr[3] <- 存在せずエラー
2. add arr[0]

この問題の修正のため、inversePatchesへの(配列に関する)パッチの追加をunshiftすることで対応する。

@shqld shqld requested review from uzimith and skobaken7 September 25, 2024 16:44
@uzimith
Copy link

uzimith commented Sep 26, 2024

@shqld
ちなみにこのPRで https://linear.app/legalscape/issue/LWB-602/st4bのitemid重複問題の調査 が解決するんでしょうか? 関係なく発見したバグですか?
(本家に戻すと発生しないとも言っていたので気になりました)

@shqld
Copy link
Author

shqld commented Sep 26, 2024

@uzimith これはitem.id重複とは関係なく、pdfaug-coreの関数にpatchesのassertionを仕込んだときに発見したものです。重複問題は #1 で修正済みです!

@uzimith
Copy link

uzimith commented Sep 26, 2024

なるほど、逆patch + patchを実行したときに発見したバグですね 理解です 🙇

@shqld
Copy link
Author

shqld commented Sep 26, 2024

(本家でも再現するので近いうちにissueを送る予定)

本家の unadlib#64 に起票しました。本家は別のロジックなのでこのPRのようにシンプルに解決しそうになかったのでPRは諦めました 🫠

Copy link

@skobaken7 skobaken7 left a comment

Choose a reason for hiding this comment

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

array以外のinversePatchesは反転させなくて大丈夫ですかね?
僕がやったときは最後に全部をreverseさせました
83f36b3#diff-acef5581de9dc1f5af0e13953013e713d536553038309cb29fc3aa30cd6bb7ceR58

@shqld
Copy link
Author

shqld commented Sep 26, 2024

なるほど 👀

inversePatchesの適用時に配列の長さが最初に復元されてさえいれば、あとのオブジェクトの変更がどうであれ(存在しないpathへの変更の)問題は起きない想定だったのですが、どちらの方がより安全なのか判断に自信がない... 🤯

@skobaken7 ちなみに全体を反転させた理由はありますか?

@skobaken7
Copy link

skobaken7 commented Sep 26, 2024

ちなみに全体を反転させた理由はありますか?

そもそも数学的?論理的?に考えて逆操作というのは逆順に適用するものなので、mutativeの元の実装が誤っているという考えです。
逆操作間に依存関係が常にない(数学的に言うと交換法則が成り立つ)ならmutativeの元の実装でもいいのですが、他の箇所で成り立っているか怪しいため、安全側に寄せるのが良い気がしています。

@shqld
Copy link
Author

shqld commented Sep 26, 2024

逆操作間に依存関係が常にない(数学的に言うと交換法則が成り立つ)ならmutativeの元の実装でもいいのですが、他の箇所で成り立っているか怪しいため、安全側に寄せるのが良い気がしています。

確かに最終的にその方針で行くのが良さそうです。

ただ、(我々の現行バージョンでは)配列の逆パッチ列に関しては順操作の逆元ではなく、順パッチ列と同じ順番で適用しても問題ないようにインデックスを調整しているので全体を反転させるには実装を変える必要がありそうです 😢

@shqld
Copy link
Author

shqld commented Sep 26, 2024

ちょっと試してみます。

@shqld
Copy link
Author

shqld commented Sep 26, 2024

おそらく本家は長さによって配列におけるpatches, inversePatchesを入れ替えているので逆パッチ列の全体反転をやりづらかったんだろうな...

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