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

chore(fix): fixes sync response not correctly identifying all errors #2346

Merged
merged 6 commits into from Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions packages/api/src/Domain/Http/HttpService.ts
Expand Up @@ -315,12 +315,14 @@ export class HttpService implements HttpServiceInterface {
console.error(error)
}
if (httpStatus >= HttpStatusCode.Success && httpStatus < HttpStatusCode.InternalServerError) {
if (
httpStatus === HttpStatusCode.Forbidden &&
response.data &&
(response as HttpErrorResponse).data.error !== undefined
) {
;(response as HttpErrorResponse).data.error.message = ErrorMessage.RateLimited
if (httpStatus === HttpStatusCode.Forbidden && isErrorResponse(response)) {
if (!response.data.error) {
response.data.error = {
message: ErrorMessage.RateLimited,
}
} else {
response.data.error.message = ErrorMessage.RateLimited
}
}
resolve(response)
} else {
Expand Down
4 changes: 2 additions & 2 deletions packages/responses/src/Domain/Error/ClientDisplayableError.ts
@@ -1,4 +1,4 @@
import { HttpErrorResponse } from '../Http'
import { HttpErrorResponse, getErrorFromErrorResponse } from '../Http'

export class ClientDisplayableError {
constructor(public text: string, public title?: string, public tag?: string) {
Expand All @@ -14,7 +14,7 @@ export class ClientDisplayableError {
}

static FromNetworkError(error: HttpErrorResponse) {
return new ClientDisplayableError(error.data.error.message)
return new ClientDisplayableError(getErrorFromErrorResponse(error).message)
}
}

Expand Down
@@ -1,5 +1,5 @@
import { HttpError } from './HttpError'

export type HttpErrorResponseBody = {
error: HttpError
error?: HttpError
}
12 changes: 12 additions & 0 deletions packages/responses/src/Domain/Http/HttpResponse.ts
Expand Up @@ -2,6 +2,7 @@ import { HttpErrorResponseBody } from './HttpErrorResponseBody'
import { HttpResponseMeta } from './HttpResponseMeta'
import { HttpHeaders } from './HttpHeaders'
import { HttpStatusCode } from './HttpStatusCode'
import { HttpError } from './HttpError'

type AnySuccessRecord = Record<string, unknown> & { error?: never }

Expand All @@ -24,3 +25,14 @@ export type HttpResponse<T = AnySuccessRecord> = HttpErrorResponse | HttpSuccess
export function isErrorResponse<T>(response: HttpResponse<T>): response is HttpErrorResponse {
return (response.data as HttpErrorResponseBody)?.error != undefined || response.status >= 400
}

export function getErrorFromErrorResponse(response: HttpErrorResponse): HttpError {
const embeddedError = response.data.error
if (embeddedError) {
return embeddedError
}

return {
message: 'Unknown error',
}
}
Expand Up @@ -8,7 +8,7 @@ export class GetInboundAsymmetricMessages {
const response = await this.messageServer.getMessages()

if (isErrorResponse(response)) {
return ClientDisplayableError.FromError(response.data.error)
return ClientDisplayableError.FromNetworkError(response)
}

return response.data.messages
Expand Down
Expand Up @@ -8,7 +8,7 @@ export class GetOutboundAsymmetricMessages {
const response = await this.messageServer.getOutboundUserMessages()

if (isErrorResponse(response)) {
return ClientDisplayableError.FromError(response.data.error)
return ClientDisplayableError.FromNetworkError(response)
}

return response.data.messages
Expand Down
Expand Up @@ -16,7 +16,7 @@ export class SendAsymmetricMessageUseCase {
})

if (isErrorResponse(response)) {
return ClientDisplayableError.FromError(response.data.error)
return ClientDisplayableError.FromNetworkError(response)
}

return response.data.message
Expand Down
4 changes: 2 additions & 2 deletions packages/services/src/Domain/Files/FileService.ts
Expand Up @@ -386,7 +386,7 @@ export class FileService extends AbstractService implements FilesClientInterface

const result = await this.api.deleteFile(tokenResult, file.shared_vault_uuid ? 'shared-vault' : 'user')

if (result.data?.error) {
if (isErrorResponse(result)) {
const deleteAnyway = await this.alertService.confirm(
spaceSeparatedStrings(
'This file could not be deleted from the server, possibly because you are attempting to delete a file item',
Expand All @@ -400,7 +400,7 @@ export class FileService extends AbstractService implements FilesClientInterface
)

if (!deleteAnyway) {
return ClientDisplayableError.FromError(result.data?.error)
return ClientDisplayableError.FromNetworkError(result)
}
}

Expand Down
8 changes: 4 additions & 4 deletions packages/services/src/Domain/Revision/RevisionManager.ts
@@ -1,6 +1,6 @@
import { RevisionApiServiceInterface } from '@standardnotes/api'
import { Uuid } from '@standardnotes/domain-core'
import { isErrorResponse } from '@standardnotes/responses'
import { getErrorFromErrorResponse, isErrorResponse } from '@standardnotes/responses'

import { InternalEventBusInterface } from '../Internal/InternalEventBusInterface'
import { AbstractService } from '../Service/AbstractService'
Expand All @@ -21,7 +21,7 @@ export class RevisionManager extends AbstractService implements RevisionClientIn
const result = await this.revisionApiService.listRevisions(itemUuid.value)

if (isErrorResponse(result)) {
throw new Error(result.data.error.message)
throw new Error(getErrorFromErrorResponse(result).message)
}

return result.data.revisions
Expand All @@ -31,7 +31,7 @@ export class RevisionManager extends AbstractService implements RevisionClientIn
const result = await this.revisionApiService.deleteRevision(itemUuid.value, revisionUuid.value)

if (isErrorResponse(result)) {
throw new Error(result.data.error.message)
throw new Error(getErrorFromErrorResponse(result).message)
}

return result.data.message
Expand All @@ -41,7 +41,7 @@ export class RevisionManager extends AbstractService implements RevisionClientIn
const result = await this.revisionApiService.getRevision(itemUuid.value, revisionUuid.value)

if (isErrorResponse(result)) {
throw new Error(result.data.error.message)
throw new Error(getErrorFromErrorResponse(result).message)
}

return result.data.revision
Expand Down
Expand Up @@ -23,7 +23,7 @@ export class SendSharedVaultInviteUseCase {
})

if (isErrorResponse(response)) {
return ClientDisplayableError.FromError(response.data.error)
return ClientDisplayableError.FromNetworkError(response)
}

return response.data.invite
Expand Down
Expand Up @@ -23,7 +23,7 @@ export class UpdateSharedVaultInviteUseCase {
})

if (isErrorResponse(response)) {
return ClientDisplayableError.FromError(response.data.error)
return ClientDisplayableError.FromNetworkError(response)
}

return response.data.invite
Expand Down
Expand Up @@ -4,7 +4,7 @@ import { InternalEventBusInterface } from '../Internal/InternalEventBusInterface
import { AbstractService } from '../Service/AbstractService'
import { SubscriptionClientInterface } from './SubscriptionClientInterface'
import { AppleIAPReceipt } from './AppleIAPReceipt'
import { isErrorResponse } from '@standardnotes/responses'
import { getErrorFromErrorResponse, isErrorResponse } from '@standardnotes/responses'

export class SubscriptionManager extends AbstractService implements SubscriptionClientInterface {
constructor(
Expand All @@ -19,7 +19,7 @@ export class SubscriptionManager extends AbstractService implements Subscription
const result = await this.subscriptionApiService.acceptInvite(inviteUuid)

if (isErrorResponse(result)) {
return { success: false, message: result.data.error.message }
return { success: false, message: getErrorFromErrorResponse(result).message }
}

return result.data
Expand Down Expand Up @@ -81,7 +81,7 @@ export class SubscriptionManager extends AbstractService implements Subscription
})

if (isErrorResponse(result)) {
return { success: false, message: result.data.error.message }
return { success: false, message: getErrorFromErrorResponse(result).message }
}

return result.data
Expand Down
10 changes: 8 additions & 2 deletions packages/services/src/Domain/User/UserService.ts
@@ -1,6 +1,12 @@
import { Base64String } from '@standardnotes/sncrypto-common'
import { EncryptionProviderInterface, SNRootKey, SNRootKeyParams } from '@standardnotes/encryption'
import { HttpResponse, SignInResponse, User, isErrorResponse } from '@standardnotes/responses'
import {
HttpResponse,
SignInResponse,
User,
getErrorFromErrorResponse,
isErrorResponse,
} from '@standardnotes/responses'
import { KeyParamsOrigination, UserRequestType } from '@standardnotes/common'
import { UuidGenerator } from '@standardnotes/utils'
import { UserApiServiceInterface, UserRegistrationResponseBody } from '@standardnotes/api'
Expand Down Expand Up @@ -227,7 +233,7 @@ export class UserService
if (isErrorResponse(response)) {
return {
error: true,
message: response.data.error.message,
message: getErrorFromErrorResponse(response).message,
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/snjs/lib/Services/Api/ApiService.ts
Expand Up @@ -664,7 +664,7 @@ export class SNApiService
})

if (isErrorResponse(response)) {
return ClientDisplayableError.FromError(response.data.error)
return ClientDisplayableError.FromNetworkError(response)
}
const data = response.data
return {
Expand Down
13 changes: 7 additions & 6 deletions packages/snjs/lib/Services/Session/SessionManager.ts
Expand Up @@ -45,6 +45,7 @@ import {
ChangeCredentialsResponse,
SessionListResponse,
HttpSuccessResponse,
getErrorFromErrorResponse,
} from '@standardnotes/responses'
import { CopyPayloadWithContentOverride, RootKeyWithKeyPairsInterface } from '@standardnotes/models'
import { LegacySession, MapperInterface, Result, Session, SessionToken } from '@standardnotes/domain-core'
Expand Down Expand Up @@ -316,7 +317,7 @@ export class SNSessionManager
const result = await this.apiService.getSubscription(this.getSureUser().uuid)

if (isErrorResponse(result)) {
return ClientDisplayableError.FromError(result.data?.error)
return ClientDisplayableError.FromNetworkError(result)
}

const subscription = result.data.subscription
Expand All @@ -328,7 +329,7 @@ export class SNSessionManager
const response = await this.apiService.getAvailableSubscriptions()

if (isErrorResponse(response)) {
return ClientDisplayableError.FromError(response.data.error)
return ClientDisplayableError.FromNetworkError(response)
}

return response.data
Expand Down Expand Up @@ -418,8 +419,8 @@ export class SNSessionManager
ephemeral,
})

if ('error' in registerResponse.data) {
throw new ApiCallError(registerResponse.data.error.message)
if (isErrorResponse(registerResponse)) {
throw new ApiCallError(getErrorFromErrorResponse(registerResponse).message)
}

await this.handleAuthentication({
Expand Down Expand Up @@ -492,8 +493,8 @@ export class SNSessionManager
const result = await this.performSignIn(email, password, strict, ephemeral, minAllowedVersion)
if (
isErrorResponse(result.response) &&
result.response.data.error.tag !== ErrorTag.ClientValidationError &&
result.response.data.error.tag !== ErrorTag.ClientCanceledMfa
getErrorFromErrorResponse(result.response).tag !== ErrorTag.ClientValidationError &&
getErrorFromErrorResponse(result.response).tag !== ErrorTag.ClientCanceledMfa
) {
const cleanedEmail = cleanedEmailString(email)
if (cleanedEmail !== email) {
Expand Down
14 changes: 7 additions & 7 deletions packages/snjs/lib/Services/Settings/SettingsGateway.ts
@@ -1,7 +1,7 @@
import { SettingsList } from './SettingsList'
import { SettingName } from '@standardnotes/settings'
import { API_MESSAGE_INVALID_SESSION } from '@standardnotes/services'
import { HttpStatusCode, isErrorResponse, User } from '@standardnotes/responses'
import { getErrorFromErrorResponse, HttpStatusCode, isErrorResponse, User } from '@standardnotes/responses'
import { SettingsServerInterface } from './SettingsServerInterface'

/**
Expand Down Expand Up @@ -34,7 +34,7 @@ export class SettingsGateway {
const response = await this.settingsApi.listSettings(this.userUuid)

if (isErrorResponse(response)) {
throw new Error(response.data?.error.message)
throw new Error(getErrorFromErrorResponse(response).message)
}

if (response.data == undefined || response.data.settings == undefined) {
Expand All @@ -53,7 +53,7 @@ export class SettingsGateway {
}

if (isErrorResponse(response)) {
throw new Error(response.data?.error.message)
throw new Error(getErrorFromErrorResponse(response).message)
}

return response?.data?.setting?.value ?? undefined
Expand All @@ -71,7 +71,7 @@ export class SettingsGateway {
}

if (isErrorResponse(response)) {
throw new Error(response.data?.error.message)
throw new Error(getErrorFromErrorResponse(response).message)
}

return response?.data?.setting?.value ?? undefined
Expand All @@ -89,7 +89,7 @@ export class SettingsGateway {
}

if (isErrorResponse(response)) {
throw new Error(response.data?.error.message)
throw new Error(getErrorFromErrorResponse(response).message)
}

return response.data?.success ?? false
Expand All @@ -98,14 +98,14 @@ export class SettingsGateway {
async updateSetting(name: SettingName, payload: string, sensitive: boolean): Promise<void> {
const response = await this.settingsApi.updateSetting(this.userUuid, name.value, payload, sensitive)
if (isErrorResponse(response)) {
throw new Error(response.data?.error.message)
throw new Error(getErrorFromErrorResponse(response).message)
}
}

async deleteSetting(name: SettingName): Promise<void> {
const response = await this.settingsApi.deleteSetting(this.userUuid, name.value)
if (isErrorResponse(response)) {
throw new Error(response.data?.error.message)
throw new Error(getErrorFromErrorResponse(response).message)
}
}

Expand Down
11 changes: 7 additions & 4 deletions packages/snjs/lib/Services/Sync/Account/Response.ts
Expand Up @@ -9,6 +9,7 @@ import {
RawSyncResponse,
UserEventServerHash,
AsymmetricMessageServerHash,
getErrorFromErrorResponse,
} from '@standardnotes/responses'
import {
FilterDisallowedRemotePayloadsAndMap,
Expand All @@ -35,8 +36,6 @@ export class ServerSyncResponse {
private successResponseData: RawSyncResponse | undefined

constructor(public rawResponse: HttpResponse<RawSyncResponse>) {
this.rawResponse = rawResponse

if (!isErrorResponse(rawResponse)) {
this.successResponseData = rawResponse.data
}
Expand Down Expand Up @@ -101,7 +100,11 @@ export class ServerSyncResponse {
}

public get error(): HttpError | undefined {
return isErrorResponse(this.rawResponse) ? this.rawResponse.data?.error : undefined
if (isErrorResponse(this.rawResponse)) {
return getErrorFromErrorResponse(this.rawResponse)
} else {
return undefined
}
}

public get status(): number {
Expand All @@ -123,6 +126,6 @@ export class ServerSyncResponse {
}

public get hasError(): boolean {
return this.error != undefined
return isErrorResponse(this.rawResponse)
}
}
Expand Up @@ -12,7 +12,7 @@ import Icon from '@/Components/Icon/Icon'
import IconButton from '@/Components/Button/IconButton'
import AdvancedOptions from './AdvancedOptions'
import HorizontalSeparator from '../Shared/HorizontalSeparator'
import { isErrorResponse } from '@standardnotes/snjs'
import { getErrorFromErrorResponse, isErrorResponse } from '@standardnotes/snjs'

type Props = {
viewControllerManager: ViewControllerManager
Expand Down Expand Up @@ -99,7 +99,7 @@ const SignInPane: FunctionComponent<Props> = ({ application, viewControllerManag
.signIn(email, password, isStrictSignin, isEphemeral, shouldMergeLocal)
.then((response) => {
if (isErrorResponse(response)) {
throw new Error(response.data?.error.message)
throw new Error(getErrorFromErrorResponse(response).message)
}
viewControllerManager.accountMenuController.closeAccountMenu()
})
Expand Down