Skip to content

Conversation

@SystemKeeper
Copy link
Contributor

Summary

The etag check is neither useful (as the computation is already done at that point), nor is it working correctly.

@SystemKeeper SystemKeeper requested a review from a team as a code owner April 29, 2025 20:17
@SystemKeeper SystemKeeper requested review from ArtificialOwl, come-nc and icewind1991 and removed request for a team April 29, 2025 20:17
@kesselb
Copy link
Contributor

kesselb commented Apr 29, 2025

If it would work, we should save a few bytes by not retransferring something the client already has 😉

@kesselb kesselb added 3. to review Waiting for reviews bug labels Apr 29, 2025
@kesselb kesselb added this to the Nextcloud 32 milestone Apr 29, 2025
@SystemKeeper
Copy link
Contributor Author

If it would work, we should save a few bytes by not retransferring something the client already has 😉

But it works already, because we set the etag on the response object, and then it is handled in https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php ? The only advantage of doing it manually would be to save computation, if I can generate the etag, without retrieving/building the data, but that is not the case here, the processing happens always.

The PHPUnit tests are failing, because we don't wire up the middleware (and also the header name needs to be changed), like in

$this->middleWare = new NotModifiedMiddleware(
$this->request
);
.
But IMO we should just remove the tests, as we are otherwise just testing what the middleware-test already tests!?

@SystemKeeper SystemKeeper force-pushed the fix/52278/remove-unused-etag-check branch from 8c67164 to f590fad Compare April 29, 2025 21:11
@SystemKeeper SystemKeeper force-pushed the fix/52278/remove-unused-etag-check branch from f590fad to 1addd35 Compare April 29, 2025 21:15
@kesselb
Copy link
Contributor

kesselb commented Apr 29, 2025

as we are otherwise just testing what the middleware-test already tests!?

Yes. It would be nice though to keep a modified version of the test to assert that the logout url is ignored. According to #8652 (comment) that url changes and is therefore excluded.

@kesselb
Copy link
Contributor

kesselb commented Apr 29, 2025

It's super weird to return the logout url in the api response. The requesttoken is bound to the current session and therefore will not work somewhere else 🤷

@SystemKeeper
Copy link
Contributor Author

@kesselb That's what you had in mind?

@kesselb kesselb merged commit be19825 into master Apr 30, 2025
194 of 200 checks passed
@kesselb kesselb deleted the fix/52278/remove-unused-etag-check branch April 30, 2025 20:28
@skjnldsv skjnldsv mentioned this pull request Aug 19, 2025
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ETag check of /navigation/apps not working

5 participants