Datastore - Queries created by Transaction don't fall back to Datastore namespace

[beaulac]

Queries created in a transaction without an explicitly specified namespace do not fall back to the datastore's constructor-specified namespace.

Because of this, the namespace must always be explicitly specified for ancestor queries created within a transaction.

This is probably not desired behaviour, since the docs for Transaction#createQuery provide a namespace-less example (transaction.createQuery('Company');)

Steps to reproduce

const ds = require('@google-cloud/datastore')({namespace: 'myNamespace', projectId: 'my-project'});

const ancestorKey = ds.key(['ancestorKind', 1234]);

// This is OK, query namespace matches key namespace:
ds.createQuery('childKind')
  .hasAncestor(ancestorKey)
  .run()
  .then(([results]) => results);

ds.transaction().run()
  .then(([activeTransaction]) => {

// This fails! 
// "Error: The query namespace is  but ancestor namespace is myNamespace"
    activeTransaction.createQuery('childKind')
             .hasAncestor(ancestorKey)
             .run()
             .then(([results]) => results);
  });

Cause

This occurs because Transaction binds this to itself when calling datastore's createQuery:

Transaction.prototype.createQuery = function() {
  return this.datastore.createQuery.apply(this, arguments);
};

Then, in createQuery:

  if (arguments.length < 2) {
    kind = namespace;
    namespace = this.namespace;
  }

Unfortunately this.namespace is undefined 😭

This could be fixed in the Transaction constructor:

function Transaction(datastore) {
  this.datastore = datastore;
// Add this line to match datastore's fallback namespace:
  this.namespace = datastore.namespace;

Environment details

Probably not too relevant, but:

• OS: macOS 10.12.5
• Node.js version: 6.10.2
• npm version: 5.1.0
• google-cloud/datastore version: 1.0.2

[callmehiphop]

Thanks for reporting! We'd be more than happy to accept a pull request if you'd like to contribute, otherwise I think this is something we can easily tackle within the week.

[beaulac]

Hi! I've opened #2448 that fixes this. I'm not sure why the CI is failing, though; I ran the tests before creating the PR and everything was fine.

[callmehiphop]

Oh hey, I didn't realize you already sent one over :)

[callmehiphop]

@stephenplusplus can you take a peek at why CI is failing? It looks like code coverage went up, but I think that's the reason the build is failing.

[beaulac]

All the tests passed on v4, but then it bailed out when calling nvm use v6. I noticed that nvm install v4 is run in the Circle dependencies phase, but not nvm install v6. I added it to test.sh in the same manner as v7 and v8. I'll revert this if you'd prefer this change be done in the Circle CI dependencies config.

Edit: Apparently adding nvm install v6 just changes the exit code from 3 to 1 😕
It would be really helpful to get the actual error from nvm instead of just a number...

[stephenplusplus]

Yeah, Circle is just an error fest right now. As long as AppVeyor is passing, this is "as good as green" until we have time to dig into Circle (probably when we revisit #2386)