[X] Added TODO flags to code segments that need to be re-reviewed to ensure that they are working as intended. Specifically, the flags are added to array concatenations that involve the spread operator such as allStatuses = [...allStatuses, ...newStatuses], which would yield unpredictable results when multi-dimensional arrays are involved.
[X] Fixed several several dozen invocations of improper ...rest property binding that involve HTTP response headers. As previously written, the resulting headers object would omit elements that followed ...cors(), in the assignment expression.
[X] Fix several bugs in reblog (ActivityPub Announce handler) business logic:
[X] Fix bug that allowed multiple AP Announce activities for the same remote AP Object
[X] Fix bug that allowed AP Announce for APObject with a null payload
[X] Fix bug that would create notification when a user re-blogs a post that they authored
[X] Fix bug that would call fetch() in an attempt to cache APObject whenever a local user re-blogs a post that they authored
[X] Fix bugs in reblog logic that would allow reblog + notification for remote objects that were inaccessible by the local instance (i.e. due to connectivity errors or de-federation/blocking by remote instance)
[X] Fix bugs in several tests that were using the wrong ID type for assertions (e.g. checking for equality of remote APObject id instead of checking for equality of local APObject id)
TL; DR:
Tests that were previously passing but are now breaking rely on assertions that validate hard-coded defaults- these values don't change when the business logic yields null or undefined values along the way, as was happening in several areas due to variable assignment and object binding using the spread and rest operators (...). Now that the correct syntax is being used and the bugs in the original code has been addressed, the proper use of ... in variable assignment should be a point of emphasis for code reviews moving forward.
👀 Please review/merge https://github.com/cloudflare/wildebeest/pull/363 before this PR
Examples of the problem:
Fix the problem
[X] Non-breaking fixes to ...spread vs. ...rest destructuring in front-end code
[X] Added
TODO
flags to code segments that need to be re-reviewed to ensure that they are working as intended. Specifically, the flags are added to array concatenations that involve the spread operator such asallStatuses = [...allStatuses, ...newStatuses]
, which would yield unpredictable results when multi-dimensional arrays are involved.[X] Fixed several several dozen invocations of improper
...rest
property binding that involve HTTP response headers. As previously written, the resulting headers object would omit elements that followed...cors(),
in the assignment expression.[X] Fixed additional variable assignments that use
...rest
property destructuring. Note: this commit breaks existing tests, which were wronglyPASS
as a result of bugs in the underlying code that were not being caught by the tests.Identify bugs caused by the problem and Fix bugs caused by the problem
Announce
handler) business logic:fetch()
in an attempt to cache APObject whenever a local user re-blogs a post that they authoredid
instead of checking for equality of local APObjectid
)TL; DR:
Tests that were previously passing but are now breaking rely on assertions that validate hard-coded defaults- these values don't change when the business logic yields
null
orundefined
values along the way, as was happening in several areas due to variable assignment and object binding using the spread and rest operators (...
). Now that the correct syntax is being used and the bugs in the original code has been addressed, the proper use of...
in variable assignment should be a point of emphasis for code reviews moving forward.References:
https://gist.github.com/yesvods/51af798dd1e7058625f4
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#copy_an_array
cc @xtuc @dario-piotrowicz