sensedeep / dynamodb-onetable

DynamoDB access and management for one table designs with NodeJS
https://doc.onetable.io/
MIT License
690 stars 109 forks source link

count: true param is not returning count value #484

Closed yousafhanif6186 closed 1 year ago

yousafhanif6186 commented 1 year ago

Describe the bug

I am using latest version of "dynamodb-onetable": "^2.6.3". I need the count of records for my query. await UserModel.find( {}, { index: 'gs1', count: true } )

This query always return empty array but if i remove count flag this returns the array of records in the table.

Debugging

I tried debugging the code and found the issue in line https://github.com/sensedeep/dynamodb-onetable/blob/5387f5df66afbc15d8d1ec9d9e07ce06cd4d00d8/src/Model.js#L440C75-L440C75

This should be changed to Object.defineProperty(items, 'count', {enumerable: true})

And at line https://github.com/sensedeep/dynamodb-onetable/blob/5387f5df66afbc15d8d1ec9d9e07ce06cd4d00d8/src/Model.js#L499

need to add results.count = items.count;

Can someone pls. review? and create a PR If this solution works.

These changes solved my problem.

PR

I have created a PR with proposed solution pls. review

https://github.com/sensedeep/dynamodb-onetable/pull/485

Environment (please complete the following information):

mobsense commented 1 year ago

When you do a {count: true}, that is converted into a DynamoDB expression Select: 'Count'. This does an item count but does not return the items. i.e. it costs somewhat less than an actual read. You still incur RCU, but not transfer costs.

So it is by design that you don't get the items.

The count is not enumerable, it it is still there.

i.e.

let list = await UserModel.find({}, {count: true})

console.log(list.count)

This should print the count. But if you try to enumerate the list items with "for", you won't have count as an item.

It is important that count be non-enumerable and by design you don't get the items.

yousafhanif6186 commented 1 year ago

Hey thank you for your response. I understood the logic. I am not expecting list of items I only need count of items.

But when i do

let list = await UserModel.find({}, {count: true})console.log(list.count)

list.count is always undefined. That’s why I applied that fix

mobsense commented 1 year ago

When I do that here, count is defined correctly.

Can you supply a test case based on the debug.ts that demonstrates.

yousafhanif6186 commented 1 year ago

@mobsense Here is my code how I am using dynamodb-onetable. Can you pls. take a look if something is wrong in my configs

`

const { Dynamo } = require('dynamodb-onetable/Dynamo');
const { DynamoDBClient } = require('@aws-sdk/client-dynamodb');
const { Table } = require('dynamodb-onetable');
const dynamodbClient = new DynamoDBClient({
    region: 'local',
    endpoint: 'http://localhost:8000'
});

const client = new Dynamo({ client: dynamodbClient });

const Schema = {
    format: 'onetable:1.1.0',
    version: '0.0.1',
    indexes: {
        primary: { hash: 'pk', sort: 'sk' },
        gs1: { hash: 'gs1pk', sort: 'gs1sk', follow: true },
        gs2: { hash: 'gs2pk', sort: 'gs2sk', follow: true },
        gs3: { hash: 'gs3pk', sort: 'gs3sk', follow: true },
        gs4: { hash: 'gs4pk', sort: 'gs4sk', follow: true },
    },
    params: {
        createdField: 'createdAt',
        updatedField: 'updatedAt',
        isoDates: true,
        timestamps: true,
    },
    models: {
        User: {
            pk: { type: String, value: '${_type}#${id}' },
            sk: { type: String, value: '${_type}#' },
            id: {
                type: String,
                required: true,
                unique: true,
                generate: 'ulid',
                validate: /^[0123456789ABCDEFGHJKMNPQRSTVWXYZ]{26}$/i,
            },
            firebaseId: { type: String, required: true, unique: true },
            email: { type: String, required: true, unique: true },
            firstName: { type: String },
            lastName: { type: String },
            phoneNumber: { type: String },
            emailPreferences: {
                type: Object,
                schema: {
                    weeklyNewsletter: { type: Date },
                },
            },
            gs1pk: { type: String, value: '${_type}#' },
            gs1sk: { type: String, value: '${_type}#${firebaseId}' },
            gs2pk: { type: String, value: '${_type}#' },
            gs2sk: { type: String, value: '${_type}#${email}' },
        }
    }
};
const table = new Table({
    client: client,
    name: 'table_name',
    schema: Schema,
    logger: true,
});

const UserModel = table.getModel('User');

const items = await UserModel.find({}, { index: 'gs1', count: true });
console.log(items.count);

// items.count is always undefined
// but it count: true is not sent in the configs I get the full list of users

`

mobsense commented 1 year ago

That works fine here.

Can you add trace in the code. In Model.run(). Add trace around the code:

            if (params.count || params.select == 'COUNT') {
                items.count = count
                Object.defineProperty(items, 'count', {enumerable: false})
            }
gregkysenko commented 1 year ago

It was tricky for me to understand that count of the empty array actually has the correct value, since I was logging this array and not seeing it until I`ve logged items.count explicitly

mobsense commented 1 year ago

Yes, that is exactly the design. You want a "for ... in" to NOT iterate over the count, but items.count is there. It is just not enumerable.