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

Allow modelKey to be a nested field #638

Closed
doceme opened this issue Feb 28, 2022 · 20 comments
Closed

Allow modelKey to be a nested field #638

doceme opened this issue Feb 28, 2022 · 20 comments
Assignees
Labels
Investigation Determine next steps P0 Must Now

Comments

@doceme
Copy link

doceme commented Feb 28, 2022

I am trying to migrate my existing project from lounge to ottoman, but my existing project stores type in a meta object. Since this is a nested field, I'm not currently able to set modelKey to meta.type without getting a Couchbase InternalServerFailureError. I would issue a pull-request, but I would need some help on where to get started with this change. I'm not sure how difficult it would be. Thanks.

@AV25242
Copy link
Contributor

AV25242 commented Feb 28, 2022

Hello @doceme thanks for reaching out, trying to understand the ask by confirming some assumptions here.

I pulled down the lounge project - https://github.com/cvent/lounge and I see that in lounge you define a base schema something like this which has a metadata and the metadata has a doc_type

 var baseSchema = lounge.schema({
  metadata: {
    doc_type: String,
    createdAt: Date,
    updatedAt: Date
  }
})

Assuming while migrating to Ottoman you have have defined in the main Schema a field metadata with the similar structure as above, what you are trying to achieve is to assign metadata.doctype to modelKey when defining your model correct ?

And, what you have tried so far is

const schema = new Schema({ name: String, age: Number, metadata : {doc_type:String} });
const myModel = model('myModel', schema, { modelKey : {metadata.doc_type} });
or
const myModel = model('myModel', schema, { modelKey : 'metadata.doc_type' });

and it doesnt work and you want to know how can I achieve it or how do I contribute a PR to make it happen

Is this correct ?

@gsi-alejandro can you please followup ?

@AV25242 AV25242 added the Investigation Determine next steps label Feb 28, 2022
@doceme
Copy link
Author

doceme commented Feb 28, 2022

Hello @AV25242. Thanks for responding so quickly. The gist of what you have described is correct, except for different field names. The lounge project doesn't have a specific requirement to use a metadata or type field, however all of the documents in my project do have a base schema like this:

var baseSchema = lounge.schema({
  meta: {
    type: String,
    created: Number, // This is a unix timestamp in seconds instead of Date
    updated: Number, // This is a unix timestamp in seconds instead of Date
  }
})

I realize that having a field named type is a little tricky, since it is used almost like a reserved keyword when building the schema in ottoman. That is not the issue, though, because I have also tried using _type in my meta field as well and get the same result.

What I have tried with ottoman is:

const bucketName = 'test';
const connectionString = 'couchbase://localhost/';
const username = 'username';
const password = 'redacted';

const collectionName = '_default';
const modelKey = 'meta._type';
const keyGeneratorDelimiter = ':';

const ottoman = new Ottoman({ collectionName, modelKey, keyGeneratorDelimiter });
await ottoman.connect({ connectionString, bucketName, username, password });

const schema = new Schema({ name: String, age: Number, meta: { _type: String } });
const MyModel = ottoman.model('myModel', schema);
const data = new MyModel({ name: 'Jane Doe', age: 20 });

await ottoman.start();
await data.save();

And if I change:

const schema = new Schema({ name: String, age: Number, meta: { _type: String } });
const modelKey = 'meta._type';

to

const schema = new Schema({ name: String, age: Number, _type: String });
const modelKey = '_type';

everything works as expected.

@AV25242
Copy link
Contributor

AV25242 commented Feb 28, 2022

Yeah that is correct type especially _type need to be avoided as its a reserved keyword. so to summarize do you still need a way to handle this or are you good and you are going to model it as per the standard i.e using _type ?

@doceme
Copy link
Author

doceme commented Feb 28, 2022

My problem is unrelated to type versus _type. If I use _type, my problem still exists. As I understand it, ottoman currently requires the _type field to be at the top level of the schema. It can't be nested under a meta object. That is what I'm trying to achieve with ottoman. It's the only thing preventing me from using ottoman at the moment. Since my project is already deployed and has lots of existing documents, I can't simply move my _type field up to the top-level. I hope that makes sense.

@AV25242
Copy link
Contributor

AV25242 commented Feb 28, 2022

Understood. Team is looking to see what can be done. Thanks for the detail explanation

@gsi-alejandro
Copy link
Collaborator

hi, @doceme

What Couchbase Server version are you using?

@doceme
Copy link
Author

doceme commented Feb 28, 2022

Hello @gsi-alejandro, we're using Couchbase Server version 7.0.3 at the moment.

@AV25242
Copy link
Contributor

AV25242 commented Mar 2, 2022

Hello @doceme the concept of modelKey creates a Global Secondary Index behind the scene, ie any model method that uses a SQL++ (former N1QL) query, such as model.find(...) adds a predicate by default something like where _type = 'modelKeyValue', _type being the default field that gets added to each document and evaluates to your model name, now this helps with any greenfield project.

modelKey ( Model Options ) gives you a flexibility to override the default i.e _type (useful for migration project ) and provide your own predicate key. In your case it would be meta.type. Since we dont currently support nested objects we are evaluating solutions that might be limited to the first level of nesting (due to the complexity and overhead when it comes to management & performance).
It is also worth noting down that modelKey is immutable i.e if you try to remove or update the modelKey from the document, Ottoman will do the validation and will ignore such operations on the modelKey.

Having said all that one workaround (in the meanwhile) for you could be to write a SQL++ update statement on all the documents that basically adds a _type field with the value of meta.type and you can continue with the migration.

On a side note I see that you are on 7.x but using _default scope/ collection, is that a transitive step and you plan to use named scope and collections ? The reason I ask is that will also change how you define Ottoman models (for instance one collection per document type) .

@doceme
Copy link
Author

doceme commented Mar 2, 2022

Hello @AV25242, thanks for your consideration of this request. I understand that ottoman creates a GSI to handle the model.find operations and I'm okay with that. We currently have a GSI for meta.type lookups that we created, but I imagine it would get replaced with the ottoman created one after the transition. I am completely okay with modelKey being immutable and would expect it to be. I think limiting the nesting depth of the modelKey is perfectly reasonable. We don't currently have any plans to migrate away from using the _default scope/collection, but I believe I understand the implications if we do down the road.

@gsi-alejandro
Copy link
Collaborator

hi @AV25242, @doceme
I already added support for nested modelKey values in this PR

It will be released in the next iteration. (dev -> master)

e.g.
const modelKey = 'metadata.doc_type';

will work for this schema

 const baseSchema = lounge.schema({
  metadata: {
    doc_type: String,
    createdAt: Date,
    updatedAt: Date
  }
})

@doceme
Copy link
Author

doceme commented Mar 14, 2022

Thanks @gsi-alejandro! I have tested this PR on my project and it fixes the issue for modelKey. However, when I try to save a document, the createdAt and updateAt fields are not stored in the metadata object. For example:

    const modelKey = 'metadata.doc_type';

    const schema = new Schema({
      name: String,
      age: Number,
      metadata: {
        doc_type: String,
        createdAt: Date,
        updatedAt: Date,
      }
    }, {
      timestamps: {
        createdAt: 'metadata.createdAt',
        updateAt: 'metadata.updatedAt',
      }
    });

    const User = ottoman.model('user', schema, { modelKey });
    const data = new User({ name: 'Jane Doe', age: 20 });

    await ottoman.start();
    const doc = await data.save();
    console.log(doc.toJSON());

and here is how a document is saved using this schema:

{
  "name": "Jane Doe",
  "age": 20,
  "metadata.createdAt": "2022-03-14T14:42:39.198Z",
  "id": "b569ef1a-ebc7-4853-8dd8-6bddcbaf5349",
  "metadata": {
    "doc_type": "user"
  }
}

Would it be possible to have createdAt and updatedAt also stored in metadata? Should I create a separate issue for this? I really appreciate your help with this.

@gsi-alejandro
Copy link
Collaborator

hi @doceme

The PR just has changes to support nested value in the modelKey.

We'll create a new ticket this week to handle timestamps the same way. (I'll prepare a proposal in order to be approved and be able to proceed into the implementation.)

I will keep you updated about timestamps with progress.

@gsi-alejandro
Copy link
Collaborator

@doceme if you want to try to build the schema this way:

  const metadataSchema = new Schema(
    {
      doc_type: String,
    },
    { timestamps: true },
  );

  const schema = new Schema({
    name: String,
    age: Number,
    metadata: metadataSchema,
  });

Note: We probably should update something else and of course add some tests for it, but should work.

@gsi-alejandro
Copy link
Collaborator

hi @doceme

I already did and test the changes need for timestamps to work in a nested schema.

The changes are in dev branch, they will be released in the next release

These schemas will work for your use case modelKey and timestamps nested in the metadata key.

  const metadataSchema = new Schema(
    {
      doc_type: String,
    },
    { timestamps: true },
  );

  const schema = new Schema({
    name: String,
    age: Number,
    metadata: metadataSchema,
  });

Notice: you can reuse metadataSchema across all your schema to build them consistently while defining it, in a very easy way.

I hope this has been helpful, cheers/

@doceme
Copy link
Author

doceme commented Mar 14, 2022

@gsi-alejandro Thanks for the suggestion. That would work if we were using Date for the createdAt and updatedAt fields, but we're actually using a Number as described here: https://ottomanjs.com/guides/schema.html#option-timestamps. I used a Date field in my previous example because the code worked to at least store a document in the database without giving an error. If I use a Number field, I get: ValidationError: Property 'createdAt' must be of type 'Date'.

@gsi-alejandro
Copy link
Collaborator

@doceme

It should support whatever timestamps valid options and combinations, therefore this schema should work as expected:

 const metadataSchema = new Schema(
      {
        doc_type: String,
        createdAt: Number,
        updatedAt: Number,
      },
      { timestamps: { currentTime: () => Math.floor(Date.now() / 1000) } },
    );

    const schema = new Schema({
      name: String,
      age: Number,
      metadata: metadataSchema,
    });

Actually, I add some tests for it and they passed.
Test implementation:
image

console output:
image

@doceme
Copy link
Author

doceme commented Mar 14, 2022

@gsi-alejandro Wonderful! Thank you so much. We've been wanting to transition our project to ottoman for a while now. This will definitely make it possible for us to do so.

@gsi-alejandro
Copy link
Collaborator

nice to hear that @doceme

If you need something else just create a ticket or if you have any ideas to improve are welcome too, we're continuously enhancing Ottoman.

Feel free to close this ticket if there aren't any remaining issues.

have a nice day, happy coding 🎉

@AV25242
Copy link
Contributor

AV25242 commented Apr 6, 2022

Hello @doceme did it help, we released it with 2.2.0. please let us know.

@doceme
Copy link
Author

doceme commented Apr 6, 2022

Hi @AV25242. Yes, we are currently in the process of migrating from lounge to ottoman using version 2.2.0. We haven't hit any show-stoppers yet. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Investigation Determine next steps P0 Must Now
Projects
None yet
Development

No branches or pull requests

3 participants