Use parent from options when constructing a Datastore Key

[yenif]

Adds parent to options for Key constructor to specify a parent of the provided path.

Thoughts?

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[yenif]

I signed it!

[googlebot]

CLAs look good, thanks!

[stephenplusplus]

Thank you for this! I like the goal, but how about instead of supplying a parent directly, we just had a key method on all Key objects?:

var company = datastore.key(['Company', 123]) // the parent
var department = company.key(['Department', 'Engineering']) // the child

// or...

var department = company.child(['Department', 'Engineering'])

[yenif]

Sure that would work. For a little context, I currently have this method on an object wrapping Datastore

    // Takes an int, string, array, or key object and returns a key object
    key(key, {parent} = {}) {
        if (typeof(key) != 'object') {
            key = [key];
        }   

        if (Array.isArray(key)) {
            key = [this.kind].concat(key);

            if(parent) {
              key = parent.path.concat(key);
            } 

            return datastore.key({
              namespace: 'test',
              path: key
            });
        } else {
            return key;         
        } 
    } 

With the original change that would be

    key(key, {parent} = {}) {
        if (typeof(key) != 'object') {
            key = [key];
        }   

        if (Array.isArray(key)) {
            key = [this.kind].concat(key);

            return datastore.key({
              namespace: 'test',
              path: key,
              parent: parent
            });
        } else {
            return key;         
        }   
    } 

With your suggestion I would have

    key(key, {parent} = {}) {
        if (typeof(key) != 'object') {
            key = [key];
        }   

        if (Array.isArray(key)) {
            key = [this.kind].concat(key);

            if(parent) {
              return parent.key({
                namespace: 'test',
                path: key
              });
            } else {
              return datastore.key({
                namespace: 'test',
                path: key
              });
            }
        } else {
            return key;         
        }   
    } 

Personal taste, I prefer the middle one.

[yenif]

I suppose the biggest potential downside of this change as is, is that currently I think this works

var company = datastore.key(['Company', 123, 'Department', 'Engineering']);
var dup_key = datastore.key(company);

dup_key.path //= ['Company', 123, 'Department', 'Engineering'];

But with the current patch I believe it would break as

var company = datastore.key(['Company', 123, 'Department', 'Engineering']);
var dup_key = datastore.key(company);

dup_key.path //= ['Company', 123, 'Company', 123, 'Department', 'Engineering']

No idea if this is intended functionality.

[stephenplusplus]

Thank you for sharing the context, that always helps. I can see the concern for the new method as there is more complexity. I tried to re-work your function to see "what I would do" (of course, this isn't what you would do), but let me know if I've maintained functionality while getting it to a less intimidating size, in a way that you might use:

function key(path, parent) {
    if (typeof path === 'object') {
        // Key provided is already a Key object.
        return path;
    }

    path = Array.isArray(path) ? path : [path];

    return (parent || datastore).key({
        namespace: 'test',
        path: [this.kind].concat(path)
    });
}

Small win by using arrify:

function key(path, parent) {
    if (typeof path === 'object') {
        // Key provided is already a Key object.
        return path;
    }

    return (parent || datastore).key({
        namespace: 'test',
        path: [this.kind].concat(arrify(path)) // with npm module arrify
    });
}

On the duplicate key note, any behavior, working or not, would be incidental. I don't believe we've considered the use case of passing a Key object into key. Let me know if you've seen that in the docs or mentioned elsewhere. At first glance, I'm not too concerned about supporting that use, but what do you think?

[yenif]

Quick follow up. My previous key normalizing function ended up biting me when I started using parent child relationships with id keys.

datastore.key(key.path) will turn a {id: '12345'} key into a {name: '123345'} which obviously breaks things. I am now explicitly setting parent on keys as follows

    // Normalize key
    // Takes an int, string, array, or key object and returns a key object
    key(key, {parent} = {}) {
        switch (typeof(key)) {
        case 'object':
            return key;
        case 'undefined':
            key = [];
            break;
        case 'string':
            if (key.match(/^\d+$/)) {
                key = datastore.int(key);
            }

            key = [key];
            break;
        case 'number':
            key = [key];
            break;
        default:
            throw 'Unhandled key type ' + typeof(key);
        }

        key = [this.kind].concat(key);

        key = datastore.key({
          namespace: DATASTORE_NAMESPACE,
          path: key
        });

        if(parent) {
          key.parent = parent;
        }

        return key;
    }

[stephenplusplus]

Due to the age of this request, I'm going to close the PR. If you're still interested in this feature, please create an issue for further discussion or a PR at the new Datastore location: https://github.com/googleapis/nodejs-datastore. Thanks!