Add REST store support #48

lzhoucs · 2016-11-15T17:23:08Z

Type: feature

Description:
Add a REST based storage that interacts with a target REST endpoint remotely. Refactored createInMemoryStorage

Related Issue: #47 , #4

Please review this checklist before submitting your PR:

There is a related issue
All contributors have signed a CLA
All code matches the style guide
The code passes the CI tests
Unit or Functional tests are included in the PR
The PR increases or maintains the overall unit test coverage percentage
The code is ready to be merged

lzhoucs · 2016-11-15T21:02:59Z

All tests passed. The build is hanging on "Created session iphone 9.3 on any platform". It doesn't seem to be caused by my changes.

maier49 · 2016-11-15T18:51:17Z

src/storage/createInMemoryStorage.ts

-			});
-		}
-	},
+const createInMemoryStorage: InMemoryStorageFactory = createBaseStorage.mixin(compose({


If we're using mixin(), is there a reason we're also using compose(), or could we just pass a mixin descriptor? e.g.

{ mixin: { createId(...) ... }, initialize(instance, options) { } }

Yeah. That's a good idea and I tried it. But it doesn't seem to work. You can find the change in this branch

Does seem like a strange usage, should work with the format bradley used above

This is now using a mixin descriptor argument instead of a factory.

maier49 · 2016-11-15T21:58:43Z

src/storage/createRestStorage.ts

+			uri += '/' + ids[0];
+		}
+		else {
+			uri += '?' + state.idSerializer(ids);


We should probably check if state.idSerializer exists and default to making individual get requests here if it doesn't.

Thanks. This makes sense to me. I will make the change here(get) as well as in delete

maier49 · 2016-11-15T21:59:20Z

src/storage/createRestStorage.ts

+			uri += '/' + ids[0];
+		}
+
+		return <Promise<any>> request(uri, {


We should probably check if state.requestSerializer exists and default to making individual put requests here if it doesn't.

This one is tricky. requestSerializer is used as the object => string serializer regardless of the type of the request. So it can't be used as an indicator of opt in custom request. I can probably add another option just for put custom request scenario, unless you have other ideas.

maier49 · 2016-11-15T22:00:30Z

src/storage/createRestStorage.ts

+	}
+}, <T, O>(instance: Storage<T, O>, {target,
+									createIdTarget,
+									idSerializer = defaultIdSerializer,


The serializers used for multiple puts and multiple gets probably shouldn't be defaulted. If they are supporting non standard ways of doing things as a convenience we will probably want to make that an opt in behavior.

I agree. I am making changes for this. For get and delete, I will repurpose idSerializer as the indicator, and for put, I will add another option, since requestSerializer is being used in multiple scenarios.

lzhoucs · 2016-11-17T08:44:26Z

I made the discussed changes. Will do a rebase once other PRs are landed.

tomdye · 2016-11-22T16:15:36Z

src/storage/createBaseStorage.ts

+			return itemArray.map(state.idFunction);
+		}
+		else {
+			return itemArray.map(function(item) {


return itemArray.map((item) => item.id);

tomdye · 2016-11-22T16:16:14Z

src/storage/createBaseStorage.ts

+		const state = instanceStateMap.get(this);
+		const itemArray = Array.isArray(items) ? <IdObject []> items : [ <IdObject> items ];
+		if (state.idProperty) {
+			return itemArray.map((item) => {


return itemArray.map((item) => item[ state.idProperty ]);

tomdye · 2016-11-22T16:22:23Z

src/storage/createRestStorage.ts

+				headers: {
+					'Accept': state.contentType
+				}
+			}).then(function(response) {


utilise the arrow functions:
.then((response) => state.responseDeserializer(response.data.toString()));

tomdye · 2016-11-22T16:25:54Z

tests/unit/storage/createRestStorage.ts

+	},
+
+	'GET': {
+		'ids can\'t be empty'() {


Can you be a bit more descriptive with your names please. ie.
Should throw error when get called without ids

tomdye · 2016-11-22T16:30:28Z

src/storage/createInMemoryStorage.ts

-			});
-		}
-	},
+const createInMemoryStorage: InMemoryStorageFactory = createBaseStorage.mixin(compose({


Does seem like a strange usage, should work with the format bradley used above

tomdye · 2016-11-22T16:34:53Z

Have only done a brief review.
Also why the usage of String(thing) all over the place over thing.toString()?

lzhoucs · 2016-11-24T02:32:02Z

Thanks @tomdye for the review. I addressed the feedback.

Regarding the String over toString(), ideally none of them should be used since response.data is already a string. However there were some typing issues, so I used the String wrapper to get around the issue in 3 places. Anyway my latest commit have addressed them.

tomdye · 2016-11-24T11:18:05Z

@lzhoucs I would imagine the typings issues you described are just that, and should be fixed via the typings? I'm probably wrong though. As a general rule we should avoid using any as much as possible.

lzhoucs · 2016-11-24T18:02:22Z

You're right @tomdye. And I wouldn't be surprised if typing issues were fixed by typing changes. As you can see from my change, I added generic typing <string> when calling request.get so response.data will be string as expected, as such there's no need to do any type of conversion as I did with String() wrapper.

As for any, I did try not to use it as much as possible. If there's any place you think I can replace any with a concrete type, please let me know and I am happy to make the change.

codecov-io · 2016-12-09T17:18:41Z

Codecov Report

Merging #48 into master will decrease coverage by 1.13%.
The diff coverage is 93.53%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage     100%   98.86%   -1.14%     
==========================================
  Files          16       18       +2     
  Lines        1166     1322     +156     
  Branches      204      241      +37     
==========================================
+ Hits         1166     1307     +141     
- Misses          0       10      +10     
- Partials        0        5       +5

Impacted Files	Coverage Δ
src/store/createQueryTransformResult.ts	`100% <ø> (ø)`	⬆️
src/query/createFilter.ts	`100% <ø> (ø)`	⬆️
src/patch/createPatch.ts	`100% <ø> (ø)`	⬆️
src/storage/createInMemoryStorage.ts	`100% <100%> (ø)`	⬆️
src/store/createStore.ts	`100% <100%> (ø)`	⬆️
src/storage/createBaseStorage.ts	`100% <100%> (ø)`
src/storage/createRestStorage.ts	`90.13% <90.13%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 900986c...31cb84c. Read the comment docs.

dylans · 2017-02-01T00:02:40Z

This one was ready to land, but we're waiting for dojo/core#261 to land and then we'll refactor this on top of that.

dylans

This will likely need some refactoring when dojo/core#261 lands.

maier49 · 2017-04-07T00:01:02Z

Closing in favor of #124 which has been rebased against the latest from master and works with the updated request API from dojo/core

lzhoucs changed the title ~~Rest storage~~ Add REST store support Nov 15, 2016

maier49 suggested changes Nov 15, 2016

View reviewed changes

dylans added this to the 2016.11 milestone Nov 16, 2016

tomdye suggested changes Nov 22, 2016

View reviewed changes

dylans modified the milestones: 2016.11, 2016.12 Dec 5, 2016

maier49 approved these changes Dec 5, 2016

View reviewed changes

maier49 mentioned this pull request Dec 14, 2016

JSON merge patch #80

Closed

7 tasks

dylans modified the milestones: 2017.01, 2016.12 Dec 20, 2016

maier49 mentioned this pull request Jan 2, 2017

Add total length to fetch result promise #94

Merged

7 tasks

Rest storage implementation

31cb84c

dylans modified the milestones: 2017.01, 2017.02 Jan 31, 2017

dylans reviewed Feb 6, 2017

View reviewed changes

dylans modified the milestones: 2017.03, 2017.02 Feb 28, 2017

dylans added this to the 2017.04 milestone Mar 29, 2017

dylans removed this from the 2017.03 milestone Mar 29, 2017

maier49 closed this Apr 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add REST store support #48

Add REST store support #48

lzhoucs commented Nov 15, 2016 •

edited by maier49

Loading

lzhoucs commented Nov 15, 2016

maier49 Nov 15, 2016

lzhoucs Nov 16, 2016

tomdye Nov 22, 2016

maier49 Dec 12, 2016

maier49 Nov 15, 2016

lzhoucs Nov 16, 2016

maier49 Nov 15, 2016

lzhoucs Nov 16, 2016

maier49 Nov 15, 2016

lzhoucs Nov 16, 2016

lzhoucs commented Nov 17, 2016

tomdye Nov 22, 2016

tomdye Nov 22, 2016

tomdye Nov 22, 2016

tomdye Nov 22, 2016

tomdye Nov 22, 2016

tomdye commented Nov 22, 2016

lzhoucs commented Nov 24, 2016

tomdye commented Nov 24, 2016 •

edited

Loading

lzhoucs commented Nov 24, 2016

codecov-io commented Dec 9, 2016 •

edited by codecov bot

Loading

dylans commented Feb 1, 2017

dylans left a comment

maier49 commented Apr 7, 2017

Add REST store support #48

Add REST store support #48

Conversation

lzhoucs commented Nov 15, 2016 • edited by maier49 Loading

lzhoucs commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhoucs commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdye commented Nov 22, 2016

lzhoucs commented Nov 24, 2016

tomdye commented Nov 24, 2016 • edited Loading

lzhoucs commented Nov 24, 2016

codecov-io commented Dec 9, 2016 • edited by codecov bot Loading

Codecov Report

dylans commented Feb 1, 2017

dylans left a comment

Choose a reason for hiding this comment

maier49 commented Apr 7, 2017

lzhoucs commented Nov 15, 2016 •

edited by maier49

Loading

tomdye commented Nov 24, 2016 •

edited

Loading

codecov-io commented Dec 9, 2016 •

edited by codecov bot

Loading