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.

Sign up for GitHub

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

fix(@aws-amplify/datastore): adds missing fields to items sent through observe/observeQuery #9973

Merged
merged 5 commits into from
Jun 13, 2022
Merged

fix(@aws-amplify/datastore): adds missing fields to items sent through observe/observeQuery #9973

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
305cc1c
manual rebase: add missing fields to items sent through observe/obser…
svidgen Jun 10, 2022
aebf936
added testing to ensure outbox only contains expected fields
svidgen Jun 10, 2022
bca754a
removed cruft; removed explicit check for undef mutation field
svidgen Jun 10, 2022
5befcd5
Merge branch 'main' into gh-9682
svidgen Jun 10, 2022
0da12b2
Merge branch 'main' into gh-9682
svidgen Jun 13, 2022
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
260 changes: 258 additions & 2 deletions packages/datastore/__tests__/DataStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,16 @@ import {
PersistentModel,
PersistentModelConstructor,
} from '../src/types';
import { Comment, Model, Post, Metadata, testSchema, pause } from './helpers';
import {
Comment,
Model,
Post,
Profile,
Metadata,
User,
testSchema,
pause,
} from './helpers';

let initSchema: typeof initSchemaType;
let DataStore: typeof DataStoreType;
Expand Down Expand Up @@ -265,13 +274,17 @@ describe('DataStore observeQuery, with fake-indexeddb and fake sync', () => {

let Comment: PersistentModelConstructor<Comment>;
let Post: PersistentModelConstructor<Post>;
let User: PersistentModelConstructor<User>;
let Profile: PersistentModelConstructor<Profile>;

beforeEach(async () => {
({ initSchema, DataStore } = require('../src/datastore/datastore'));
const classes = initSchema(testSchema());
({ Comment, Post } = classes as {
({ Comment, Post, User, Profile } = classes as {
Comment: PersistentModelConstructor<Comment>;
Post: PersistentModelConstructor<Post>;
User: PersistentModelConstructor<User>;
Profile: PersistentModelConstructor<Profile>;
});

// This prevents pollution between tests. DataStore may have processes in
Expand Down Expand Up @@ -598,6 +611,249 @@ describe('DataStore observeQuery, with fake-indexeddb and fake sync', () => {
}
})();
});

test('attaches related belongsTo properties consistently with query() on INSERT', async done => {
try {
const expecteds = [5, 15];

for (let i = 0; i < 5; i++) {
await DataStore.save(
new Comment({
content: `comment content ${i}`,
post: await DataStore.save(
new Post({
title: `new post ${i}`,
})
),
})
);
}

const sub = DataStore.observeQuery(Comment).subscribe(
({ items, isSynced }) => {
const expected = expecteds.shift() || 0;
expect(items.length).toBe(expected);

for (let i = 0; i < expected; i++) {
expect(items[i].content).toEqual(`comment content ${i}`);
expect(items[i].post.title).toEqual(`new post ${i}`);
}

if (expecteds.length === 0) {
sub.unsubscribe();
done();
}
}
);

setTimeout(async () => {
for (let i = 5; i < 15; i++) {
await DataStore.save(
new Comment({
content: `comment content ${i}`,
post: await DataStore.save(
new Post({
title: `new post ${i}`,
})
),
})
);
}
}, 1);
} catch (error) {
done(error);
}
});

test('attaches related hasOne properties consistently with query() on INSERT', async done => {
try {
const expecteds = [5, 15];

for (let i = 0; i < 5; i++) {
await DataStore.save(
new User({
name: `user ${i}`,
profile: await DataStore.save(
new Profile({
firstName: `firstName ${i}`,
lastName: `lastName ${i}`,
})
),
})
);
}

const sub = DataStore.observeQuery(User).subscribe(
({ items, isSynced }) => {
const expected = expecteds.shift() || 0;
expect(items.length).toBe(expected);

for (let i = 0; i < expected; i++) {
expect(items[i].name).toEqual(`user ${i}`);
expect(items[i].profile.firstName).toEqual(`firstName ${i}`);
expect(items[i].profile.lastName).toEqual(`lastName ${i}`);
}

if (expecteds.length === 0) {
sub.unsubscribe();
done();
}
}
);

setTimeout(async () => {
for (let i = 5; i < 15; i++) {
await DataStore.save(
new User({
name: `user ${i}`,
profile: await DataStore.save(
new Profile({
firstName: `firstName ${i}`,
lastName: `lastName ${i}`,
})
),
})
);
}
}, 1);
} catch (error) {
done(error);
}
});

test('attaches related belongsTo properties consistently with query() on UPDATE', async done => {
try {
const expecteds = [
['old post 0', 'old post 1', 'old post 2', 'old post 3', 'old post 4'],
['new post 0', 'new post 1', 'new post 2', 'new post 3', 'new post 4'],
];

for (let i = 0; i < 5; i++) {
await DataStore.save(
new Comment({
content: `comment content ${i}`,
post: await DataStore.save(
new Post({
title: `old post ${i}`,
})
),
})
);
}

const sub = DataStore.observeQuery(Comment).subscribe(
({ items, isSynced }) => {
const expected = expecteds.shift() || [];
expect(items.length).toBe(expected.length);

for (let i = 0; i < expected.length; i++) {
expect(items[i].content).toContain(`comment content ${i}`);
expect(items[i].post.title).toEqual(expected[i]);
}

if (expecteds.length === 0) {
sub.unsubscribe();
done();
}
}
);

setTimeout(async () => {
let postIndex = 0;
const comments = await DataStore.query(Comment);
for (const comment of comments) {
const newPost = await DataStore.save(
new Post({
title: `new post ${postIndex++}`,
})
);

await DataStore.save(
Comment.copyOf(comment, draft => {
draft.content = `updated: ${comment.content}`;
draft.post = newPost;
})
);
}
}, 1);
} catch (error) {
done(error);
}
});

test('attaches related hasOne properties consistently with query() on UPDATE', async done => {
try {
const expecteds = [
[
'first name 0',
'first name 1',
'first name 2',
'first name 3',
'first name 4',
],
[
'new first name 0',
'new first name 1',
'new first name 2',
'new first name 3',
'new first name 4',
],
];

for (let i = 0; i < 5; i++) {
await DataStore.save(
new User({
name: `user ${i}`,
profile: await DataStore.save(
new Profile({
firstName: `first name ${i}`,
lastName: `last name ${i}`,
})
),
})
);
}

const sub = DataStore.observeQuery(User).subscribe(
({ items, isSynced }) => {
const expected = expecteds.shift() || [];
expect(items.length).toBe(expected.length);

for (let i = 0; i < expected.length; i++) {
expect(items[i].name).toContain(`user ${i}`);
expect(items[i].profile.firstName).toEqual(expected[i]);
}

if (expecteds.length === 0) {
sub.unsubscribe();
done();
}
}
);

setTimeout(async () => {
let userIndex = 0;
const users = await DataStore.query(User);
for (const user of users) {
const newProfile = await DataStore.save(
new Profile({
firstName: `new first name ${userIndex++}`,
lastName: `new last name ${userIndex}`,
})
);

await DataStore.save(
User.copyOf(user, draft => {
draft.name = `updated: ${user.name}`;
draft.profile = newProfile;
})
);
}
}, 1);
} catch (error) {
done(error);
}
});
});

describe('DataStore tests', () => {
Expand Down
44 changes: 26 additions & 18 deletions packages/datastore/src/datastore/datastore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1145,24 +1145,32 @@ class DataStore {
handle = this.storage
.observe(modelConstructor, predicate)
.filter(({ model }) => namespaceResolver(model) === USER)
.map(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is truly no longer needed, then it makes sense to revert ALL of the changes I made here. PR: #9617

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all changes; your other PR ported over the initial set of missing observe/observeQuery. That's highly valuable!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing this!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested your changes, and it looks like on update, the first snapshot still preserves all fields.

(event: InternalSubscriptionMessage<T>): SubscriptionMessage<T> => {
// The `element` returned by storage only contains updated fields.
// Intercept the event to send the `savedElement` so that the first
// snapshot returned to the consumer contains all fields.
// In the event of a delete we return `element`, as `savedElement`
// here is undefined.
const { opType, model, condition, element, savedElement } = event;

return {
opType,
element: savedElement || element,
model,
condition,
};
}
)
.subscribe(observer);
.subscribe({
next: async item => {
// the `element` doesn't necessarily contain all item details or
// have related records attached consistently with that of a query()
// result item. for consistency, we attach them here.

let message = item;

// as lnog as we're not dealing with a DELETE, we need to fetch a fresh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typo

// item from storage to ensure it's fully populated.
if (item.opType !== 'DELETE') {
const freshElement = await this.query(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make sure we're not sending ALL fields to AppSync with this change? For instance, when I implemented the map above, I sent element and not savedElement when the savedElement was not present to ensure we weren't sending all fields (including those that have not been updated) to AppSync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I've added a test for this in commonAdapterTest.ts. It was a minor PITA to sort out how to construct a test for this. But, now that I have a reasonable pattern for the test, if there are other specific scenarios you have in mind to cover, we should definitely get them covered. I didn't see any other tests that would give me the confidence to address this questions otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested your changes locally, and it appears that the outgoing request is only sending updated fields 👍🏻

item.model,
item.element.id
);
message = {
...message,
element: freshElement as T,
};
}

observer.next(message as SubscriptionMessage<T>);
},
error: err => observer.error(err),
complete: () => observer.complete(),
});
})();

return () => {
Expand Down