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

datastore: support streams with request.get #755

Merged
merged 1 commit into from
Jul 31, 2015

Conversation

stephenplusplus
Copy link
Contributor

RE: #750 (comment)

This adds streamRouter to the get method from Datastore, as talked about in #750.

Here's a problem, though. streamRouter was intended for handling multiple results, like the results of a query where the user doesn't know how many to expect back. Always giving the user an array of the results to their callback makes sense there, but with dataset.get, the user knows exactly how many responses they will get back. So, that makes this awkward:

var key = dataset.key(['Company', 123]);

// Before:
dataset.get(key, function(err, entity, apiResponse) {
  // entity = the entity obtained from the key
});

// After:
dataset.get(key, function(err, entities, apiResponse) {
  // entities = array of results (will have 1 result from the 1 key)
  // entities[0] = the entity obtained from the key
});

On the bright side, the user can now get streaming results from this method:

dataset.key(keys).on('data', function(entity) {});

I haven't thought of a great solution to the problem above yet. I'm leaning towards saying streamRouter isn't the right solution in its current form. Ideas welcome :)

@stephenplusplus stephenplusplus added enhancement api: datastore Issues related to the Datastore API. labels Jul 28, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 28, 2015
@callmehiphop
Copy link
Contributor

// begin crazy talk

I agree that streamRouter in its current state probably isn't the way to go. At first I was going to say that we should just make a separate method for streams since this api is a bit different than our typically streamRouter use case -- however, I think a similar problem may already exist (well, whether or not it's actually a problem is probably up for debate)

Typically there's an option to set the maximum number of results to return, so what's the expectation when that max is set to 1 (not sure how likely this is to happen). As it is today, even though we only request 1 result, we still return an array -- this of course makes total sense, but it might offer slight inconvenience if the user is only expecting 1 result.

What if we were to add logic that checked if the max was 1 and if so, returned the singular item instead of an array. After which maybe it would be possible to somehow set the max to 1 if we saw that this particular method only received a single key?

¯_(ツ)_/¯

@rekow
Copy link

rekow commented Jul 29, 2015

This is super awesome to see!! Still want an issue for tracking or is the comment ref enough?

This might be naive, but couldn't you continue to unpack the single-item array much like in the current implementation of get in the case where a single key is passed? Fetching a single item seems mutually exclusive with pagination, so there shouldn't be any case where streamRouter runs the request as a stream if it's passed a callback - in that world there's no reason entities needs to be an array in the makeReq_ callback, right?

Just seems a bit easier than handling the unpack at the streamRouter level - then you'd have to update other streamed methods to potentially expect a single value as well, I think.

@stephenplusplus
Copy link
Contributor Author

@davidrekow no worries about the issue :)

Typically there's an option to set the maximum number of results to return, so what's the expectation when that max is set to 1 (not sure how likely this is to happen). As it is today, even though we only request 1 result, we still return an array -- this of course makes total sense, but it might offer slight inconvenience if the user is only expecting 1 result.

I think the road of least surprises is where we want to go. If we can document our method as always returning an array, nobody would expect to get an unboxed array under any conditions. Your suggestion is a pretty creative way to solve the problem, though, and it would work if we wanted our other APIs that accept maxResults to unbox the results for the callback. I think that would create unexpected results, however.

I'm leaning towards just handling the stream logic in this method individually, and avoiding using streamRouter altogether. Maybe as we go on with our library, we'll find other similar places and we can start to find ways to abstract the logic.

@stephenplusplus stephenplusplus force-pushed the spp--datastore-get branch 2 times, most recently from 85f00ae to b396f0f Compare July 30, 2015 02:30
@stephenplusplus
Copy link
Contributor Author

Let me know how :( my latest commit is.

@callmehiphop
Copy link
Contributor

I think for the sake of API consistency, it's undoubtedly a step in the right direction. What do you think the likeliness of this issue popping up again is? I only ask because I could see it resurfacing elsewhere, so maybe we'd want to put this logic into its own module

@stephenplusplus
Copy link
Contributor Author

I only ask because I could see it resurfacing elsewhere, so maybe we'd want to put this logic into its own module

It definitely might. I can't think of any off hand like this that aren't already using streamRouter, though. If the time comes when we can re-use this logic, we can module out?

I'll start on tidying up this PR.

@stephenplusplus stephenplusplus changed the title datastore: add streamrouter to request.get datastore: support streams with request.get Jul 31, 2015
@stephenplusplus stephenplusplus force-pushed the spp--datastore-get branch 2 times, most recently from f80ff71 to 46ac928 Compare July 31, 2015 15:12
@stephenplusplus
Copy link
Contributor Author

PTAL :)

callmehiphop added a commit that referenced this pull request Jul 31, 2015
datastore: support streams with request.get
@callmehiphop callmehiphop merged commit c324c8e into googleapis:master Jul 31, 2015
@callmehiphop
Copy link
Contributor

Looks good, thanks!

sofisl pushed a commit that referenced this pull request Jan 17, 2023
* chore: run th generator, fix synth script

* test: remove comment
sofisl pushed a commit that referenced this pull request Jan 24, 2023
[PR](googleapis/gapic-generator-typescript#878) within
updated gapic-generator-typescript version 1.4.0

Committer: @summer-ji-eng
PiperOrigin-RevId: 375759421

Source-Link: googleapis/googleapis@95fa72f

Source-Link: googleapis/googleapis-gen@f40a343
sofisl pushed a commit that referenced this pull request Jan 25, 2023
[PR](googleapis/gapic-generator-typescript#878) within
updated gapic-generator-typescript version 1.4.0

Committer: @summer-ji-eng
PiperOrigin-RevId: 375759421

Source-Link: googleapis/googleapis@95fa72f

Source-Link: googleapis/googleapis-gen@f40a343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants