sensedeep / dynamodb-onetable

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

New "Required field not defined" error is thrown when getting by keys only indexes #538

Open AlexStansfield opened 2 months ago

AlexStansfield commented 2 months ago

Noticed the size of my logs went up about about 50x (no joke).

We have some operations that get thousands of records by an index that is keys only. For every record it spits out an error for every missing required field. For example one request that gets only 200 records, created almost 1000 error messages.

Schema For the Index:

    [ETechIndexes.GET_BY_BATCH]: {
      hash: 'getByBatchPk',
      sort: 'getByBatchSk',
      project: 'keys',
      follow: true,
    },

Error: Required field "companyId" in model "Tech" not defined in table item

{
   "model":"Tech",
   "raw":{
      "sk":"Tech#5c479b58-abb0-4158-bf4e-c2332f44940a",
      "getByBatchPk":"Batch#9d37c4a4-f2d7-48bb-9a0f-96a6c2797627",
      "pk":"Company#908d1390-a9a5-474a-9807-76283d7eb443:Tech",
      "getByBatchSk":"Tech:000000001"
   },
   "params":{
      "parse":true,
      "high":true,
      "index":"getByBatch",
      "follow":false,
      "hidden":true,
      "checked":true
   },
   "field":{
      "type":"string",
      "required":true,
      "name":"companyId",
      "isoDates":true,
      "partial":false,
      "attribute":[
         "companyId"
      ],
      "nulls":false
   }
}

I saw there is a noerror param. Will that turn off all errors or just this "not defined" one? As I still want to catch real errors but currently the size of the logs is starting to cost us money as we pay for Kloudmate by the amount of logs ingested.

Otherwise I might have to revert to an older version until this is resolved.

I do feel like this error shouldn't have just been turned on by default, there could have been some flag to switch it on for now that could have been defaulted to on in a later version once any teething problems were resolved.

AlexStansfield commented 2 months ago

Checked the code and noerror seems to only affect this error. However it doesn't work if set in schema, only if set on the find operation itself.

AlexStansfield commented 2 months ago

So this version of the library went onto our production on Wednesday. In the last 24 hours it has produced 480,000 log messages.

My thoughts on this new error:

  1. Turn it off by default
  2. Make it a warning rather than an error
  3. Remove noerror param as naming wise could cover a lot of things
  4. Add a new param (e.g warnOnMissingRequired) that is false by default that could be set in params on schema or on operation

In a later release (maybe next major) the warnOnMissingRequired could default to true.

mobsense commented 2 months ago

Sounds like a good suggestion.

We should add to the test to not perform for indexes with projection. But your suggestion to reverse the test to be opt-in would be better.

mobsense commented 2 months ago

There are already a few places in the code that emit warnings if table.warn is true. So we can extend this.

With the proposed patch, you can set:

If set (and a whole host of conditions are not true), then a warning will be omitted.

This warning is really important if you update your schema and make some fields required, but don't update all items in the database to define the required field. You want to detect that condition. So that is what the warning is for.

With opt-in, as you suggest, you can turn it on globally via the schema or on a per-API basis.

Thanks for raising the issue.

mobsense commented 3 weeks ago

Before releasing this, did this patch address the issue?

AlexStansfield commented 3 weeks ago

Before releasing this, did this patch address the issue?

Which patch?

mobsense commented 3 weeks ago

The current repo includes the params.warn and turns warnings off by default.