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

Error in parseModel -> normalize indexes #126

Closed
calebwilson706 opened this issue Jun 10, 2022 · 8 comments
Closed

Error in parseModel -> normalize indexes #126

calebwilson706 opened this issue Jun 10, 2022 · 8 comments

Comments

@calebwilson706
Copy link

Screenshot 2022-06-10 at 13 11 43

We are getting this error message when trying to put an item in to our database, from some debugging and logs we can see that when looping through the access patterns that there are extra access patterns after our defined ones which are return undefined for the index and hence throwing the issue above, some cloudwatch logs below show the extra "access patterns"

Screenshot 2022-06-10 at 13 11 02

@tywalch
Copy link
Owner

tywalch commented Jun 10, 2022

Hi @calebwilson706

Thank you for reaching out! Does your model throw when using the electrodb playground? If so, would you be able to provide a playground like that demonstrates the issue? If not, would you be able to provide the version you are using, and a model that causes this exception?

@calebwilson706
Copy link
Author

It doesn't throw out when using the playground, I'll get you my model and version now, thanks!

@calebwilson706
Copy link
Author

We are on version 1.8.4

@calebwilson706
Copy link
Author

Our model is as follows (i have taken out any attribute which is not used for our indexes)

export const buildPartitionKey = (docmailMailingGuid: string): string => `${MAILING_PREFIX}${docmailMailingGuid}`;
export const buildSortKey = (): string => `${MAILING_PREFIX}`;
export const buildGsi1PartitionKey = (policyId: string): string => `${POLICY_PREFIX}${policyId}`;
export const buildGsi1SortKey = (effectiveDate: string): string => `${EFFECTIVE_DATE_PREFIX}${effectiveDate}`;
export const buildGsi2PartitionKey = (requestedAt: string): string => `${REQUESTED_AT_PREFIX}${requestedAt}`;
export const buildGsi2SortKey = (): string => `${REQUESTED_AT_PREFIX}`;

export const documentPrintRequest = new Entity(
  {
    model: {
      entity: "document print requests",
      version: "1",
      service: "docmail"
    },
    attributes: {
      partitionKey: {
        type: "string",
        required: true,
        readOnly: true
      },
      sortKey: {
        type: "string",
        required: true,
        readOnly: true,
        default: MAILING_PREFIX
      },
      gsi1PartitionKey: {
        type: "string",
        required: true,
        readOnly: true
      },
      gsi1SortKey: {
        type: "string",
        required: true,
        readOnly: true,
      },
      gsi2PartitionKey: {
        type: "string",
        required: true,
        readOnly: true
      },
      gsi2SortKey: {
        type: "string",
        required: true,
        readOnly: true,
        default: REQUESTED_AT_PREFIX
      },
      policyId: {
        type: "string",
        required: true
      },
      effectiveDate: {
        type: "string",
        required: true
      },
      docmailMailingGuid: {
        type: "string",
        required: true
      },
      requestedAt: {
        type: "string",
        required: true
      },
    },
    indexes: {
      byDocmailMailingGuid: {
        pk: {
          field: "partitionKey",
          composite: ["docmailMailingGuid"],
          casing: "none",
          template: buildPartitionKey("${docmailMailingGuid}")
        },
        sk: {
          field: "sortKey",
          composite: [],
          casing: "none",
          template: buildSortKey()
        }
      },
      byPolicyIdAndEffectiveDate: {
        index: "gsi1",
        pk: {
          field: "gsi1PartitionKey",
          composite: ["policyId"],
          template: buildGsi1PartitionKey("${policyId}"),
          casing: "none"
        },
        sk: {
          field: "gsi1SortKey",
          composite: ["effectiveDate"],
          template: buildGsi1SortKey("${effectiveDate}"),
          casing: "none"
        }
      },
      byRequestedAtDate: {
        index: "gsi2",
        pk: {
          field: "gsi2PartitionKey",
          composite: ["requestedAt"],
          template: buildGsi2PartitionKey("${requestedAt}"),
          casing: "none"
        },
        sk: {
          field: "gsi2SortKey",
          composite: [],
          template: buildGsi2SortKey(),
          casing: "none"
        }
      }
    }
  },
  {
    table: documentPrintRequestTableName,
    client: documentClient
  }
);```

@calebwilson706
Copy link
Author

calebwilson706 commented Jun 10, 2022

Im also having an issue when trying to complete a put operation with an attribute which is a list i'm getting a validation error

error {
    "errorType": "ElectroValidationError",
    "errorMessage": "Invalid value type at entity path \"documentsRequested[*]. Received value of type \"unknown\", expected value of type \"object\", Invalid value type at entity path \"documentsRequested[*]. Received value of type \"unknown\", expected value of type \"object\", Invalid value type at entity path \"documentsRequested[*]. Received value of type \"unknown\", expected value of type \"object\" - For more detail on this error reference: https://github.com/tywalch/electrodb#invalid-attribute",
}

once again this doesn't occur in the electrodb playground, the only differences between the playground and our code being

1.) ours is running in an actual lambda
2.) our code is split across a few files

@tywalch
Copy link
Owner

tywalch commented Jun 10, 2022

@calebwilson706 I took the steps to reproduce, installing electrodb at the version you specified (npm install electrodb@1.8.4) and made a file with your model above, but was unable to recreate the exception you shared. In looking at the error stack and its associated code, I would expect this to be throwing when your model is loaded.

In the cloudwatch logs you shared it looks like the indexes are finding async functions for forEachAsync, mapAsync, and removeNull, which suggest some of the values on the model might not be what is expected. I assume these logs are the result of printing the i, accessPattern and index values found at this line?

I completely understand the need to sanitize models, though I think some of the dynamic implementation of the models might be at play here. The playground uses the same version as you have reported, so if you are able to recreate the issue there (even with "fake" attributes) that would be very helpful to help diagnose further.


Regarding the ElectroValidationError you posed, would you be able to share the attribute definition used for this attribute? Additionally, here is a link to the code associated with identifying/validating the type of data received.

I cannot speak for the type of data being passed (the value is not logged or captured in the error to avoid leaking private data) but it does seem to be working as designed: the value received was not of the expected type. For example, a function could be flagged as unknown given the above rules.

The best course of action would be to add logging or further validation near the use of this put.

I'll leave this ticket open to maintain communication with you 👍

@calebwilson706
Copy link
Author

calebwilson706 commented Jun 13, 2022

Hello, sorry for the delay - we have found this source of our issue. We have some helper methods which modify the array prototype to give us some extension method helpers

interface Array<T> {
  forEachAsync<T>(this: Array<T>, action: (value: T) => Promise<void>): Promise<void>;
  mapAsync<T, U>(this: Array<T>, map: (value: T) => Promise<U>): Promise<U[]>;
  removeNull(this: Array<T>): Array<NonNullable<T>>;
}

Array.prototype.forEachAsync = async function forEachAsync<T>(this: Array<T>, action: (value: T) => Promise<void>): Promise<void> {
  for (const element of this) {
    await action(element);
  }
};

Array.prototype.mapAsync = async function mapAsync<T, U>(this: Array<T>, mapFunction: (value: T) => Promise<U>): Promise<U[]> {
  const result: U[] = [];
  await this.forEachAsync(async (x) => {
    result.push(await mapFunction(x));
  });
  return result;
};

Array.prototype.removeNull = function removeNull<T>(this: Array<T>): Array<NonNullable<T>> {
  return this.filter((value): value is NonNullable<T> => value != null);
};

because there is a for in loop in line 2193 of entity.js these are being found when looping. Changing this to a forEach would most likely solve the issue. I am imagining something similar is happening with the validation error discussed above also.

from the mozilla docs

Array indexes are just enumerable properties with integer names and are otherwise identical to general object properties. The for...in loop will traverse all integer keys before traversing other keys, and in strictly increasing order, making the behavior of for...in close to normal array iteration. However, the for...in loop will return all enumerable properties, including those with non–integer names and those that are inherited. Unlike for...of, for...in uses property enumeration instead of the array's iterator. In sparse arrays, for...of will visit the empty slots, but for...in will not.

It is better to use a for loop with a numeric index, Array.prototype.forEach(), or the for...of loop, because they will return the index as a number instead of a string, and also avoid non-index properties.

Thanks!

@calebwilson706
Copy link
Author

calebwilson706 commented Jun 13, 2022

i can update anywhere the for in loop is used if you would like, i'm not seeing any guide on contributing

@tywalch tywalch closed this as not planned Won't fix, can't repro, duplicate, stale Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants