Auto generated id in cloudant is not a number - todo example not working with cloudant

cloudwheels commented 6 years ago

Description / Steps to reproduce / Feature proposal

Deploy todo example connected to cloudant database using connector
Add some todos
get todos as a list
get todos by id

Current Behavior

inserted autogenerated ids are returned as a the numeric part of the document id if it starts with one, so that: database records with ids:

id"3e8428d6ba9feb6819cfb30756d3d310"
{
 "_id": "3e8428d6ba9feb6819cfb30756d3d310",
 "_rev": "3-9c0030f442f6ac47aab94ceb359179c8"
}
id"70f03fa5007cbbc02690a1c65b3f3e78"
{
 "_id": "70f03fa5007cbbc02690a1c65b3f3e78",
 "_rev": "1-8ec76b46879aa8c3147debb9ab4dedf3"
}
id"75b55970bb16205e4c377da12d97bcf7"
{
 "_id": "75b55970bb16205e4c377da12d97bcf7",
 "_rev": "1-e3f8eca5e6afbac64b52ccc1aee1ec1d"
}
id"b54ad55e77d3c0c5cda2805731cfede0"
{
 "_id": "b54ad55e77d3c0c5cda2805731cfede0",
 "_rev": "1-f03a33fff19afa3f3464ff9a6b05d43d"
}
id"e096ae6ef623c7925d2d76716013bbdc"
{
 "_id": "e096ae6ef623c7925d2d76716013bbdc",
 "_rev": "1-62a17081d83c506b0763fc9d28c5df95"
}

are returned as below by the api's GET todos:

[
    {
        "id": 3,
        "title": "get better at this"
    },
    {
        "id": 70,
        "title": "try again"
    },
    {
        "id": 75,
        "title": "add"
    },
    {
        "id": null,
        "title": "get milk"
    },
    {
        "id": null,
        "title": "test post",
        "desc": "testing post",
        "isComplete": false
    }
]

GET todos/{id} doesn't work (404 not found)

Expected Behavior

GET todos/{id} returns a todo with id={id}

Id need to be handled by loopback as a string to return the cloudant doc id.

cloudwheels commented 6 years ago

Keeping you busy on a Friday @dhmlau - so sorry ;) GET todos/{CLOUDANT_ACTUAL_DOC_ID} doesn't work (invalid data type of course as expecting a number) Not quite a simple as changing the type of id in the todo model I don't think but I have limited understanding of how id types work at this stage: just looking for something that works if you can help!!!

cloudwheels commented 6 years ago

Worryingly I'm not sure how this could ever have worked....

dhmlau commented 6 years ago

@cloudwheels, no worries :) If you're using the todo example as-is, you'd need to specify the id for POST. i.e. something like:

{
  "id": 1,
  "title": "task1",
  "desc": "aa",
  "isComplete": false
}

I just verified and it seems to be working fine for me. The document id in cloudant is the id you specify in the json.

Having said that, I did try your scenario where id is not specified in POST, and got a similar result. It is strange that some id are null and some aren't.

@raymondfeng @b-admike, any idea?

cloudwheels commented 6 years ago

@dhmlau It is (for me) a big worry / showstopper because, as with any RDB type situation, I just want (usually) a newly incremented unique id and not to have to check if it already exists etc and then supply my own, or run the risk of overwriting an existing document or duplicating a 'unique' id. I should be able to effectively manage referential integrity of the model behind my API, wrap series of posts up as transactions etc on the basis of knowing an insert (post) generates and returns a unique id if I want one, right??

So I would be very happy / prefer use cloudant doc ids as my objects' keys rather than numbers but alternatively is there, for example a get /set maxCurrentId+1 type of function available or how would I implement that without retrieving all objects in a get request and processing the id fields to get a (hopefully) unique id by the time I actually insert in another post request? Lots of overhead.

It would be a shame to revert to using a sql database just because it could generate integer ids/keys for the api models when no-sql seems to suit the architecture better.

Does it matter that cloudant is really returning an 'internal' variable of '_id' rather than a property that's been set on the model? The nulls are ids with non numeric values at the start, so the app / connector is attempting to cast the document id to an int and doing so partially successfully without throwing an error, which is interesting in itself.

Changing type of id in the todo model to 'string' caused multiple test failures but as I say I don't yet know how the special id:true type works with non sql databases / non-numeric keys, nor done any fishing for examples of working cloudant connector with loopback3 examples.

Again, I don't really feel I can progress unless I'm able to insert (post) and get a valid autogenerated id back, so this is quite important/urgent to me!

cloudwheels commented 6 years ago

I am assuming the app https://mylb4app.eu-gb.mybluemix.net/openapi.json is your deployment @dhmlau ? I can post to this without specifying id and get a numeric id back but I think it suffers the same problem as I cannot get by id and some ids are long ints some short etc? Is this app persisting to cloudant?

dhmlau commented 6 years ago

My instance is https://todo-diana.mybluemix.net/. :)
Got that from @b-admike that autogenerate id can be achieved by generated: true, see https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#id-properties.

UPDATE: Simply setting generated: true doesn't work. I think I'm missing something. Will need to look into it some more tonight. Sorry about the roadblocks that you've been encountering!

cloudwheels commented 6 years ago

If I POST to your instance with an id (=1) I get an error, id can't be set:

{
  "error": {
    "statusCode": 422,
    "name": "ValidationError",
    "message": "The `Todo` instance is not valid. Details: `id` can't be set (value: \"1\").",
    "details": {
      "context": "Todo",
      "codes": {
        "id": [
          "absence"
        ]
      },
      "messages": {
        "id": [
          "can't be set"
        ]
      }
    }
  }
}

cloudwheels commented 6 years ago

I was just about to write: I would also really like to know why your explorer page works and I just get a 'possible CORS error'. Hope to fix this when the locally hosted explorer package is released but still want to know!

So I visit my instance's explorer link which has been giving the CORS error ( https://explorer.loopback.io/?url=https://lb4.eu-gb.mybluemix.net/openapi.json ) and it's suddenly just working! Did somebody somewhere just do something? Gotta love the bleeding edge :) :) :)

cloudwheels commented 6 years ago

Highly likely the secret is in reading https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#id-properties but it's late here in the UK and if I woke up and found someone had just made it work OOTB that would be super quite frankly.

On Fri, Nov 16, 2018 at 10:24 PM Diana Lau notifications@github.com wrote:

My instance is https://todo-diana.mybluemix.net/. :) Got that from @b-admike https://github.com/b-admike that autogenerate id can be achieved by generated: true, see https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#id-properties .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/strongloop/loopback-next/issues/2040#issuecomment-439547762, or mute the thread https://github.com/notifications/unsubscribe-auth/AqZFOHsgVmbZ2Tp8TuP_g3N1Q_zxgV91ks5uvzscgaJpZM4Ymn8f .

dhmlau commented 6 years ago

@cloudwheels , my apologies again for the roadblocks you're hitting! I was trying out different things on my app on IBM Cloud, so it may not be behaving as expected. It's now at a working state: https://todo-diana.mybluemix.net.

After discussing with @b-admike, we have a few conclusions:

id is expected when creating a todo. And it's working as expected.
According to https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#id-properties:

If an ID property has generated set to true, the connector decides what type to use for the auto-generated key. For example for SQL Server, it defaults to number.

So in the case of Cloudant, id is a string. To get this example working for Cloudant, we can simply make the id type to be string, which you've tried before.
However, IMHO, the id type should be database agnostic. We've explored to use any type, but it seems like OpenAPI doesn't support this type (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#data-types). I'd like to bring it forward to the LB maintainers and see what's the best approach for this. Seems like we have a similar issue in mongodb connector as well (https://github.com/strongloop/loopback-connector-mongodb/issues/165).
For incrementing the id or specific attribute when creating the document in Cloudant, someone had done that in my previous project. It's through the design document. I can see if I can still find it, if you're interested. :)

Hope it helps!

@strongloop/loopback-maintainers, any insights?
Sorry if I've mixed up how the id related attributes in model works.

dhmlau commented 6 years ago

FYI - my updated todo example can be found here: https://github.com/dhmlau/loopback4-example-todo

cloudwheels commented 6 years ago

@dhmlau roads are pretty boring without the odd blockage, provided they an be shifted. The red lines node was throwing at me are less confusing after sleep and altering the id in the model to string type, updating the controller methods to accept string type and modifying todo controller unit tests is a couple of minutes and all works fine with cloudant, including posting without an id and getting that back as a new cloudant doc id which successfully retrieves the item by id from todos/{id}.

I think this is as it should be and saying id is a required field is fine in a unit testing sense for the application but should really fail UAT as in the real world we are effectively using this as CRUD interface for which inserting a record and retrieving a unique id back on success is a key requirement.

Also reading the actual generated openapi.json the id field seems only to be 'Required' when making a PUT to /todos/{id} not when POSTing to /todos although that maybe my misunderstanding of the openapi schema.

For a more OOTB solution perhaps we can look into how the connector exposes how it handles id fields and pass / inject the relevant type to the model and tests as a variable, or a more hacky option of some conditional logic around typeof id.

I will put up a repo at some point but working live instance at time of writing is as before, currently unauthenticated so feel free to post: https://lb4.eu-gb.mybluemix.net/openapi.json

Breakfast first though :)

cloudwheels commented 6 years ago

In summary this works as expected with the id property on the todo model set as string and test updates. ie.

post /todos with no 'id' > get back a generated string uuid of the cloudant document (its internal _id field)
post with a string 'id' > cloudant sets that as the document id
post the same string id > cloudant possible duplicate error
post with a numeric id > type error, expects string

So I can 'force' the id field ion the database by default. There is some reference to this in the cloudant connector docs here which suggests best practice is to not force the id and rely on the generated document id as the object's key, as I had intended.

This obviously has implications:

if I then wanted to use a sql connector which only provided auto generated numeric ids it might be complicated, so perhaps true database agnosticity is not acheivable
some work re-mapping existing/imported tabular related records to these ids rather than using existing primary keys (some messing with composite 'table.id' keys & flipping between tall/narrow & short/wide data structures) However, this is really just forcing my hand in aligning with a noSql persistence model (unique id per record/object instance across the whole repository) vs Sql's tablePerModelPrototype.uniqueIdForInstancesAcrossThisTableOnly structure, which I think / hope is probably the right thing to do architecturally, albeit that I may be heading down a one-way street out of sql town in doing so (good!). (I know there is some data import tooling available but haven't had a chance to look so apologies if this is all covered elsewhere). I will now also start working with the todoList example to see how changing to string type id affects relationships between the loopback models & db records.

With the possible exception of my comment above:

For a more OOTB solution perhaps we can look into how the connector exposes how it handles id fields and pass / inject the relevant type to the model and tests as a variable, or a more hacky option of some conditional logic around typeof id

This Issue is 'solved' but I think the todo example (deploy to IBM Cloud) and docs (including best practice for non-sql vs. sql?) need updating as the cloudant connection does not IMHO currently work correctly in the example without the modifications above @dhmlau

bajtos commented 5 years ago

For a more OOTB solution perhaps we can look into how the connector exposes how it handles id fields and pass / inject the relevant type to the model and tests as a variable, or a more hacky option of some conditional logic around typeof id

This Issue is 'solved' but I think the todo example (deploy to IBM Cloud) and docs (including best practice for non-sql vs. sql?) need updating as the cloudant connection does not IMHO currently work correctly in the example without the modifications above.

Makes sense to me :+1:

@cloudwheels Would you like to contribute this improvement yourself?

stale[bot] commented 4 years ago

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

stale[bot] commented 4 years ago

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

loopbackio / loopback-next