Trailing slash on realm URL can happen, and breaks /events
#3567
Comments
|
Ohhh -- it's that we're just not getting events. First clue to this was on looking in the debug console, seeing this happening over and over: But wait, why was I seeing updates if I navigated out of the message list and then back to it? ... It's because we were doing a new fetch. I'm not entirely sure why that is -- don't we avoid those if we're all caught up? the log shows we had I thought I'd spotted an update another time, too, while still looking at the message list. Maybe I'd somehow provoked another fetch? Or maybe I'd just confused that with seeing an update after navigating away and back. Anyway, that certainly explains all the symptoms. Next question: what's in turn causing it. |
|
Puzzling. Odd facts include:
|
|
Aha! The error seems to be... in the case where it's failing, the request is going to a URL like I think this naturally happens if the And, indeed, the Redux log shows that is the case for the Phew. Well, that won't be too hard to resolve, then. |
|
OK, at this point there are two puzzles, both of which go to the question of how widespread this is.
In any case, no longer a critical priority, because it's hard to trigger unless you have an install that's over two years old. I might not be the only one, though -- we should do something to clean these up. |
gnprice
added
a-data-sync
|
@timabbott Do you know a reason why this might have started failing recently? Summarizing what I found above:
Some change in our nginx and/or load-balancer setup, perhaps? |
|
Things I expect we'll want to do to clean this up:
|
|
Woah, that's weird. I am not aware of server-side changes that might have caused this; So those should behave the same. A Tornado upgrade might be the source; |
gnprice
changed the title
/events
|
Huh, odd, then. Well -- I guess that will stay a mystery, then. Most likely it's been this way for some months, which at least means the fact nobody noticed before is a good sign that this is affecting very few people. |
gnprice
added
the
a-onboarding
This sounds pretty straightforward, if those are the only required changes. |
…unts. Fixes zulip#3567. A trailing slash on a realm URL interferes with correct /events calls. Single trailing slashes were already handled; this handles multiple. The previous commit handles multiple trailing slashes only on fresh URL inputs. A migration is needed for users whose cached realm URLs have multiple.
…unts. Fixes zulip#3567. A trailing slash on a realm URL interferes with /events calls. Single trailing slashes were already handled; this handles multiple. The previous commit handles multiple trailing slashes only on fresh URL inputs. A migration is needed for users whose cached realm URLs have multiple.
…unts. Fixes: zulip#3567. A trailing slash on a realm URL interferes with /events calls. The parent commit handles multiple trailing slashes only on fresh URL inputs. A migration is needed for users whose cached realm URLs have multiple. Greg points out that we will want to take input from the server on what the canonical realm URL is, and use that.
|
Hmm, while working on #4034, I found that, apparently, we're accessing the social auth endpoints with a double slash, every time —
— and it works fine in production, but I get a 404 with a dev server. (Why the difference, I wonder?) This isn't related to how the realm URL is stored in Redux (mostly what this issue is about, I think), but I'm posting this here in case it leads to clues about the following oddness, which I think still hasn't been fully explained?
Here's where the extra slash is getting added, in where |
|
We should probably be using a proper library for joining URLs rather than string concatenation. |
Makes sense. I just opened #4081 for this, linking to your comment and a few others along these lines. |
`.login_url` on objects in `external_authentication_method` has always started with a leading slash, as far as I've seen. That means we're constantly sending requests to the server that look wrong, like https://chat.zulip.org//accounts/login/social/google . Production servers, apparently, are happy to pretend that it's a single slash and handle it with no problems; my local server in dev gives a 404. [1] In any case, we shouldn't be using weird URLs like this at all; it's likely that we wouldn't see bugs like this if we were using a proper library for joining URLs, instead of string concatenation; that's issue zulip#4081. But zulip#4081 is blocked on RN v0.62, so, do this minimal fix in the meantime. [1]: zulip#3567 (comment)
|
(Opened an interim fix for the duplicated slashes issue in social auth login endpoints as #4082.) |
`.login_url` on objects in `external_authentication_method` has always started with a leading slash, as far as I've seen. That means we're constantly sending requests to the server that look wrong, like https://chat.zulip.org//accounts/login/social/google . Production servers, apparently, are happy to pretend that it's a single slash and handle it with no problems; my local server in dev gives a 404. [1] In any case, we shouldn't be using weird URLs like this at all; it's likely that we wouldn't see bugs like this if we were using a proper library for joining URLs, instead of string concatenation; that's issue zulip#4081. But zulip#4081 is blocked on RN v0.62, so, do this minimal fix in the meantime. [1]: zulip#3567 (comment)
|
We just had a report of this last week, in #4070. AFAIK that's the first report we've heard from a user other than me. On the other hand... this user sure doesn't sound like they've been using the app since 2017 and just now hit the issue, so that means it is possible for a new user (not only a very old one) to run into in real life. And because it hits when you first log in, and makes the app just basically not work at all, it's likely to be under-reported due to people simply giving up. So, we should fix it. Here's the steps I described above:
|
Makes sense. I've got #3819 open for this, awaiting review at your convenience. |
@chrisbobbe Do you mean the contrast between
Ah, thanks for the reminder >_> I'll take a look now. |
Right.
Ah, OK! It sounds like either this solves the mystery you mention here —
— or you were referring to a different mystery. In any case, it sounds relevant. |
|
Ah, yeah. The really mysterious thing to me was what changed on the server -- the same account where things weren't working and that caused me to report this bug, I'd used in the past and it must have worked before. (But I wasn't sure how recently I might have used that particular test account on that device and seen things working, which helped make it hard to pin down.) |
`.login_url` on objects in `external_authentication_method` has always started with a leading slash, as far as I've seen. That means we're constantly sending requests to the server that look wrong, like https://chat.zulip.org//accounts/login/social/google . Production servers, apparently, are happy to pretend that it's a single slash and handle it with no problems; my local server in dev gives a 404. [1] In any case, we shouldn't be using weird URLs like this at all; it's likely that we wouldn't see bugs like this if we were using a proper library for joining URLs, instead of string concatenation; that's issue zulip#4081. [1]: zulip#3567 (comment)
`.login_url` on objects in `external_authentication_method` has always started with a leading slash, as far as I've seen. That means we're constantly sending requests to the server that look wrong, like https://chat.zulip.org//accounts/login/social/google . Production servers, apparently, are happy to pretend that it's a single slash and handle it with no problems; my local server in dev gives a 404. [1] In any case, we shouldn't be using weird URLs like this at all; it's likely that we wouldn't see bugs like this if we were using a proper library for joining URLs, instead of string concatenation; that's issue #4081. [1]: #3567 (comment)
TODO file a bug for this; find corresponding reports and complaints, to reply to them when it's fixed. We have a bug where trying to open a notification doesn't actually navigate to the conversation. I think this may currently affect all notifications. The bug is a bit like a revival of zulip#3567: we get the error "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash `/`, while the `realm_uri` value in the notification doesn't. On further inspection, it looks like this was introduced when we started using a URL object for `realm` values, in 865914f. Previously, since the fix for zulip#3567, we'd stripped trailing slashes from `realm` values, which were URL strings. But: > new URL('https://example').toString() 'https://example/' parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is *add* a trailing slash to a URL like our realm URLs (or in general, fill in the path as `/` if empty.) When a `realm_uri` with no slash is compared to one of those, it never matches. Fix the issue by doubling down on parsing as URL objects. As a side effect, this normalizes case in the URL's host (and scheme). We'd previously discussed doing that, at zulip#3671 and here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm.20URL/near/795201 and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriate `URL` implementation handy, but now we do. :-)
This fixes #4290, a regression in the last release, where trying to open a notification doesn't actually navigate to the conversation. The bug is a bit like a revival of #3567: we get the error "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash `/`, while the `realm_uri` value in the notification doesn't. On further inspection, it looks like this was introduced when we started using a URL object for `realm` values, in 865914f. Previously, since the fix for #3567, we'd stripped trailing slashes from `realm` values, which were URL strings. But: > new URL('https://example').toString() 'https://example/' parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is *add* a trailing slash to a URL like our realm URLs (or in general, fill in the path as `/` if empty.) When a `realm_uri` with no slash is compared to one of those, it never matches. Fix the issue by doubling down on parsing as URL objects. As a side effect, this normalizes case in the URL's host (and scheme). We'd previously discussed doing that, at #3671 and here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm.20URL/near/795201 and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriate `URL` implementation handy, but now we do. :-) Fixes: #4290
Thanks as always to our kind volunteer translators. i18n: Sync recently-added message strings across languages. webview build: Spell stdin as `-` for reading rsync filter rules. On Windows (in Git Bash) there's no /dev/stdin; but this works instead, as we learned here: https://chat.zulip.org/#narrow/stream/48-mobile/topic/issue/near/1047294 (Things still aren't working there as a whole, but we seem to get past one error and reach another one.) Conversely, this exact construct `--filter='. -'` appears in an example in the rsync man page, even at the ancient rsync 2.6.9 that Apple provides on macOS. Suggested-by: Anders Kaseorg <[email protected]> README: Migrate Travis badge to travis-ci.com. Signed-off-by: Anders Kaseorg <[email protected]> android notif: Correctly stringify pmUsers to fix navigation to group PMs. Navigation to a group PM on pressing a notification was broken because pmUsers was incorrectly stringified in GroupPm.getPmUsersString. E.g., for a group PM among user IDs 13313, 13434, and 13657, it would stringify to (newline added for readability): "GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657])" It should instead stringify to "13313, 13434, 13657". (Later in this series of commits, we remove the space.) Fix and add a test. notif tests: Ensure tests pass with representative pm_users values. To be reverted in the next commit. In the previous commit, we changed the return value of GroupPm.getPmUsersString in our Kotlin code from garbage separated by ', ' to numbers separated by ', '. This commit aims to prove that ', '-separated numbers will be handled correctly, at least as far as our tests can tell. But we really want it to be ','-separated (no space), which we do in the next commit. notif: Separate ids in pm_users for group PMs with ',' instead of ', '. ', ' would have been handled correctly, but seemingly by accident; in getNarrowFromNotificationData, pm_users was split on ',' to give ['1', ' 2', ' 3'] (note the spaces), then each element of that array was converted to a number. Also, replace the confusing + syntax, as in +idStrs[i], with parseInt. logging jsdoc: Move "see also" before parameters, to fix parse. When writing a call to a function that has jsdoc, VS Code shows a handy popup with the documentation. It shows first the text for the parameter you're currently typing, then the text for the function as a whole. That popup was showing the "See also" as part of the last parameter's documentation, rather than that for the function as a whole. In particular this means it was only visible when typing the last parameter. Fix the jsdoc parse, by moving everything that isn't part of a parameter's documentation to before the first @param marker. notif: Normalize realm_uri by parsing it as a URL. This fixes zulip#4290, a regression in the last release, where trying to open a notification doesn't actually navigate to the conversation. The bug is a bit like a revival of zulip#3567: we get the error "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash `/`, while the `realm_uri` value in the notification doesn't. On further inspection, it looks like this was introduced when we started using a URL object for `realm` values, in 865914f. Previously, since the fix for zulip#3567, we'd stripped trailing slashes from `realm` values, which were URL strings. But: > new URL('https://example').toString() 'https://example/' parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is *add* a trailing slash to a URL like our realm URLs (or in general, fill in the path as `/` if empty.) When a `realm_uri` with no slash is compared to one of those, it never matches. Fix the issue by doubling down on parsing as URL objects. As a side effect, this normalizes case in the URL's host (and scheme). We'd previously discussed doing that, at zulip#3671 and here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm.20URL/near/795201 and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriate `URL` implementation handy, but now we do. :-) Fixes: zulip#4290 UserItem [nfc]: Take user as one structured object. This will help us switch from emails to user IDs in downstream bits of code. Also adjust several of these call sites to use user IDs for `key`, rather than emails. UserItem [nfc]: Pass whole user to callback, rather than email. This allows UserItem call sites whose callbacks are ready to work in terms of user IDs to do so without workarounds. At the same time, passing whole user objects rather than *just* IDs allows other call sites to continue to use emails without similar, inverse workarounds. notif: Always sort user IDs in pm_users. We already ensure this in the Android case (in FcmMessage.kt); do so in the iOS case too, and document it in the type. In practice the list should already have always been sorted: the server sends it in that form, and has always done so since the pm_users field was introduced in server commit 1.7.0-2360-g693a9a5e7. (To see this in the history, try the following Git commands: git log -L :get_message_payload:zerver/lib/push_notifications.py git log -L :huddle_users:zerver/lib/message.py .) So the only way this could have gone wrong is if a rogue server changed that behavior for some reason; and the main effect of this commit is really just to document this invariant. narrow [nfc]: Document more details on identifying group PMs. Which turned up a couple of bugs! We'll fix those later in this series. example data: Take sender and recipients as pmMessage arguments. As demonstrated, this allows callers to customize these a lot more cleanly than they can by overriding the actual message properties directly. There are a few call sites we don't update here, in narrowsReducer-test.js; that file hasn't yet been upgraded to be well-typed, and so those call sites don't have real User objects to provide. example data [nfc]: Use cleaner workaround for Flow "unsealed" issue. We discovered this nicer one after having used the other one here. Reminded of the contrast in discussion on other changes in this file: zulip#4294 (comment) types: Make some more indexer-using object types inexact. I just ran into this issue with CaughtUpState when making another change. Apply the workaround there and on the remaining example in this file, and mark all instances with a conditional TODO.
[Original title: "Message list won't update, on iOS on physical device". Better characterized below after some debugging.]
AFAIK this doesn't appear in any actual release we've made -- but it happens on 25.7.121 and 25.8.122 (aka current master), in their respective alpha release builds, on my iPhone 7 running iOS 12.3. So we can't make a release until it's fixed.
It also happens in a debug build on that same device.
It doesn't happen in a debug build on an iPhone XR simulator running iOS 12.4... nor on an iPhone 7 simulator running iOS 12.4.
The symptoms are:
The text was updated successfully, but these errors were encountered: