Skip to content

Better JSON messages #8379

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

Merged
merged 6 commits into from
Jul 24, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/data-connect/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
## UNRELEASED
* Updated reporting to use @firebase/data-connect instead of @firebase/connect
* Added functionality to retry queries and mutations if the server responds with UNAUTHENTICATED.

* Updated errors to only show relevant details to the user.
17 changes: 12 additions & 5 deletions packages/data-connect/src/network/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,15 @@ export function dcFetch<T, U>(
} catch (e) {
throw new DataConnectError(Code.OTHER, JSON.stringify(e));
}
const message = getMessage(jsonResponse);
if (response.status >= 400) {
logError(
'Error while performing request: ' + JSON.stringify(jsonResponse)
);
if (response.status === 401) {
throw new DataConnectError(
Code.UNAUTHORIZED,
JSON.stringify(jsonResponse)
);
throw new DataConnectError(Code.UNAUTHORIZED, message);
}
throw new DataConnectError(Code.OTHER, JSON.stringify(jsonResponse));
throw new DataConnectError(Code.OTHER, message);
}
return jsonResponse;
})
Expand All @@ -87,3 +85,12 @@ export function dcFetch<T, U>(
return res as { data: T; errors: Error[] };
});
}
interface MessageObject {
message?: string;
}
function getMessage(obj: MessageObject): string {
if ('message' in obj) {
return obj.message;
}
return JSON.stringify(obj);
}
2 changes: 1 addition & 1 deletion packages/data-connect/test/emulatorSeeder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export async function setupQueries(
})
}
},
// eslint-disable-line camelcase
// eslint-disable-next-line camelcase
connection_string:
'postgresql://postgres:secretpassword@localhost:5432/postgres?sslmode=disable'
};
Expand Down
6 changes: 6 additions & 0 deletions packages/data-connect/test/post.gql
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,10 @@ query listPosts @auth(level: PUBLIC) {
content
}
}
query listPosts2 {
posts {
id,
content
}
}

20 changes: 16 additions & 4 deletions packages/data-connect/test/queries.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ const SEEDED_DATA = [
content: 'task 2'
}
];
const REAL_DATA = SEEDED_DATA.map(obj => ({
...obj,
id: obj.id.replace(/-/g, '')
}));
function seedDatabase(instance: DataConnect): Promise<void> {
// call mutation query that adds SEEDED_DATA to database
return new Promise((resolve, reject) => {
Expand Down Expand Up @@ -100,7 +104,7 @@ describe('DataConnect Tests', async () => {
const taskListQuery = queryRef<TaskListResponse>(dc, 'listPosts');
const taskListRes = await executeQuery(taskListQuery);
expect(taskListRes.data).to.deep.eq({
posts: SEEDED_DATA
posts: REAL_DATA
});
});
it(`instantly executes a query if one hasn't been subscribed to`, async () => {
Expand All @@ -121,7 +125,7 @@ describe('DataConnect Tests', async () => {
);
const res = await promise;
expect(res.data).to.deep.eq({
posts: SEEDED_DATA
posts: REAL_DATA
});
expect(res.source).to.eq(SOURCE_SERVER);
});
Expand All @@ -138,7 +142,7 @@ describe('DataConnect Tests', async () => {
const result = await waitForFirstEvent(taskListQuery);
const serializedRef: SerializedRef<TaskListResponse, undefined> = {
data: {
posts: SEEDED_DATA
posts: REAL_DATA
},
fetchTime: Date.now().toLocaleString(),
refInfo: {
Expand All @@ -149,7 +153,7 @@ describe('DataConnect Tests', async () => {
name: taskListQuery.name,
variables: undefined
},
source: SOURCE_SERVER
source: SOURCE_CACHE
};
expect(result.toJSON()).to.deep.eq(serializedRef);
expect(result.source).to.deep.eq(SOURCE_CACHE);
Expand All @@ -167,6 +171,14 @@ describe('DataConnect Tests', async () => {
'ECONNREFUSED'
);
});
it('throws an error with just the message when the server responds with an error', async () => {
const invalidTaskListQuery = queryRef<TaskListResponse>(dc, 'listPosts2');
const message =
'unauthorized: you are not authorized to perform this operation';
await expect(
executeQuery(invalidTaskListQuery)
).to.eventually.be.rejectedWith(message);
});
});
async function waitForFirstEvent<Data, Variables>(
query: QueryRef<Data, Variables>
Expand Down
4 changes: 2 additions & 2 deletions packages/data-connect/test/unit/dataconnect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import { ConnectorConfig, getDataConnect } from '../../src';

describe('Data Connect Test', () => {
it('should throw an error if `projectId` is not provided', async () => {
const app = initializeApp({});
const app = initializeApp({ projectId: undefined }, 'a');
expect(() =>
getDataConnect({ connector: 'c', location: 'l', service: 's' })
getDataConnect(app, { connector: 'c', location: 'l', service: 's' })
).to.throw(
'Project ID must be provided. Did you pass in a proper projectId to initializeApp?'
);
Expand Down
57 changes: 57 additions & 0 deletions packages/data-connect/test/unit/fetch.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/**
* @license
* Copyright 2024 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { expect, use } from 'chai';
import chaiAsPromised from 'chai-as-promised';
import * as sinon from 'sinon';

import { dcFetch, initializeFetch } from '../../src/network/fetch';
use(chaiAsPromised);
function mockFetch(json: object): void {
const fakeFetchImpl = sinon.stub().returns(
Promise.resolve({
json: () => {
return Promise.resolve(json);
},
status: 401
} as Response)
);
initializeFetch(fakeFetchImpl);
}
describe('fetch', () => {
it('should throw an error with just the message when the server responds with an error with a message property in the body', async () => {
const message = 'Failed to connect to Postgres instance';
mockFetch({
code: 401,
message
});
await expect(
dcFetch('http://localhost', {}, {} as AbortController, null)
).to.eventually.be.rejectedWith(message);
});
it('should throw a stringified message when the server responds with an error without a message property in the body', async () => {
const message = 'Failed to connect to Postgres instance';
const json = {
code: 401,
message1: message
};
mockFetch(json);
await expect(
dcFetch('http://localhost', {}, {} as AbortController, null)
).to.eventually.be.rejectedWith(JSON.stringify(json));
});
});
6 changes: 3 additions & 3 deletions packages/data-connect/test/unit/queries.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('Queries', () => {
const authProvider = new FakeAuthProvider();
const rt = new RESTTransport(options, undefined, authProvider);
await expect(rt.invokeQuery('test', null)).to.eventually.be.rejectedWith(
JSON.stringify(json)
json.message
);
expect(fakeFetchImpl.callCount).to.eq(2);
});
Expand All @@ -79,7 +79,7 @@ describe('Queries', () => {
const authProvider = new FakeAuthProvider();
const rt = new RESTTransport(options, undefined, authProvider);
await expect(rt.invokeMutation('test', null)).to.eventually.be.rejectedWith(
JSON.stringify(json)
json.message
);
expect(fakeFetchImpl.callCount).to.eq(2);
});
Expand All @@ -90,7 +90,7 @@ describe('Queries', () => {
rt._setLastToken('initial token');
await expect(
rt.invokeQuery('test', null) as Promise<unknown>
).to.eventually.be.rejectedWith(JSON.stringify(json));
).to.eventually.be.rejectedWith(json.message);
expect(fakeFetchImpl.callCount).to.eq(1);
});
});
Loading