Patches for v2 #716
Patches for v2 #716
Conversation
Not actually setting application/json correctly on response, which silently transitioned to parsing as text. Fixes: https://mattermost.atlassian.net/browse/MM-59860
lieut-data
added
2: Dev Review
| @@ -86,8 +86,8 @@ func NewAPI(p *Plugin, store store.Store) *API { | |||
|
|
|||
| // returnJSON writes the given data as json with the provided httpStatus | |||
| func (a *API) returnJSON(w http.ResponseWriter, data any) { | |||
| w.WriteHeader(http.StatusOK) | |||
There was a problem hiding this comment.
From https://pkg.go.dev/net/http:
Changing the header map after a call to [ResponseWriter.WriteHeader] has no effect unless the HTTP status code was of the 1xx class or the modified headers are trailers.
I think I broke this a while back when fixing a different endpoint.
| @@ -879,31 +879,23 @@ func (a *API) siteStats(w http.ResponseWriter, r *http.Request) { | |||
| http.Error(w, "unable to get whitelisted users count", http.StatusInternalServerError) | |||
| return | |||
| } | |||
| receiving, err := a.p.store.GetActiveUsersReceivingCount(metricsActiveUsersRange) | |||
| @@ -222,7 +221,7 @@ func TestProcessLifecycle(t *testing.T) { | |||
| Resource: "mockResource", | |||
| ChangeType: "mockChangeType", | |||
| ClientState: "mockClientState", | |||
| LifecycleEvent: "mockLifecycleEvent", | |||
| LifecycleEvent: "reauthorizationRequired", | |||
There was a problem hiding this comment.
Use real events, since we now check and discard unrecognized ones.
| @@ -40,6 +40,12 @@ func (c *configuration) ProcessConfiguration() { | |||
| c.ClientSecret = strings.TrimSpace(c.ClientSecret) | |||
| c.EncryptionKey = strings.TrimSpace(c.EncryptionKey) | |||
| c.WebhookSecret = strings.TrimSpace(c.WebhookSecret) | |||
| if c.MaxSizeForCompleteDownload < 0 { | |||
There was a problem hiding this comment.
There's a bug with the system console where values can sometimes not get saved, making it brutal to keep losing the various secrets. If this happens for these values, just use sensible values.
There was a problem hiding this comment.
So what are we actually fixing here, some default values that were missing? Anyone looking at the bug you mention?
There was a problem hiding this comment.
I'm trying to work around https://community.mattermost.com/core/pl/o78pcxfj87f4frt3qhrqxw1x1y, specifically.
| ah.plugin.GetMetrics().ObserveLifecycleEvent(event.LifecycleEvent, metrics.DiscardedReasonFailedToRefresh) | ||
| return | ||
| } | ||
| if event.LifecycleEvent != "reauthorizationRequired" { |
There was a problem hiding this comment.
Check the event type upfront.
| @@ -256,6 +256,9 @@ func (p *Plugin) start(isRestart bool) { | |||
| if err != nil { | |||
| p.API.LogError("failed to start metrics job", "error", err) | |||
| } | |||
|
|
|||
| // Run the job above right away so we immediately populate metrics. | |||
| go p.updateMetrics() | |||
There was a problem hiding this comment.
We'd always get dips otherwise:
| @@ -294,7 +297,7 @@ func (p *Plugin) start(isRestart bool) { | |||
| p.checkCredentialsJob = checkCredentialsJob | |||
|
|
|||
| // Run the job above right away so we immediately populate metrics. | |||
| p.checkCredentials() | |||
| go p.checkCredentials() | |||
| @@ -540,6 +543,16 @@ func (p *Plugin) GetRemoteID() string { | |||
| } | |||
|
|
|||
| func (p *Plugin) updateMetrics() { | |||
| defer func() { | |||
There was a problem hiding this comment.
This was run in an implicit goroutine already, so just tracking it like a proper worker.
|
|
||
| err = s.getQueryBuilder(db). | ||
| Select("count(*)"). | ||
| Select("count(mmUserID)"). |
There was a problem hiding this comment.
Stop COUNT(*) which requires a table scan.
| From(usersTableName). | ||
| Where(sq.GtOrEq{"LastChatReceivedAt": now.Add(-dur).UnixMicro()}). | ||
| Where(sq.LtOrEq{"LastChatReceivedAt": now.UnixMicro()}). |
Co-authored-by: Caleb Roseland <caleb@calebroseland.com>
Summary
A handful of fixes to reported and observed issues after the bug bash:
OnConfigurationChangefails, it often clears out otherwise good values for various fields. This is super painful for the admin, so just reset to sensible values instead.Ticket Link
Fixes: https://mattermost.atlassian.net/browse/MM-59876
Fixes: https://mattermost.atlassian.net/browse/MM-59862
Fixes: https://mattermost.atlassian.net/browse/MM-59860
Fixes: https://mattermost.atlassian.net/browse/MM-59859
Fixes: https://mattermost.atlassian.net/browse/MM-59861