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

[BUG] string Slug incorrectly parsed as Date if in a specific format #161

Closed
Bnaya opened this issue Jul 15, 2019 · 6 comments · Fixed by #178
Closed

[BUG] string Slug incorrectly parsed as Date if in a specific format #161

Bnaya opened this issue Jul 15, 2019 · 6 comments · Fixed by #178

Comments

@Bnaya
Copy link

Bnaya commented Jul 15, 2019

I have a controller that looks like so:

@Crud({
  model: {
    type: Product,
  },
  params: {
    id: {
      field: "id",
      type: "string",
      primary: true,
    },
  },

When GET ing product with id like so: /api/products/product-123123
The SQL query ends with:

WHERE `Brand`.`bt_brand_id` = ?) `distinctAlias` ORDER BY `Brand_bt_brand_id` ASC LIMIT 100 -- PARAMETERS: ["0122-12-31T21:39:06.000Z"]

When the ID format is not STRING-NUMBER, its all file.

I assume the lib has some heuristics, that are incorrect?
Maybe it's here:

private parseValue(val: any) {

@Bnaya
Copy link
Author

Bnaya commented Jul 15, 2019

After additional debug, the issue is here:

validateParamOption(this._paramsOptions, name);
const option = this._paramsOptions[name];
const value = this.parseValue(this._params[name]);
switch (option.type) {
case 'number':

Even tho the type is configured as string, this.parseValue parses it as Date.
parseValue should get optional expected type, and only if it's not there, should fallback to heuristic parsing.

@Bnaya
Copy link
Author

Bnaya commented Jul 15, 2019

My Workaround:

// workaround for https://github.com/nestjsx/crud/issues/161
const m = require("@nestjsx/util/lib/checks.util.js");
const origIsDateString = m.isDateString;
m.isDateString = (v: any) => {
  if (typeof v === "string") {
    if (/^[a-z]+-[0-9]+$/i.test(v)) {
      return false;
    }
  }

  return origIsDateString.apply(m, [v]);
};

@sgrebnov
Copy link

sgrebnov commented Jul 16, 2019

Same issue - I use the following filter "?filter=someParam||cont||CG-7" and "CG-7" is automatically resolved to date time

@Bnaya
Copy link
Author

Bnaya commented Jul 16, 2019

More up to date workaround:

// workaround for https://github.com/nestjsx/crud/issues/161
const m = require("@nestjsx/util/lib/checks.util.js");
const origIsDateString = m.isDateString;

// eslint-disable-next-line no-useless-escape
const datePattern = /^(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2}(?:\.\d*)?)(Z|([+\-])(\d{2}):(\d{2}))$/;

m.isDateString = (v: any) => {
  if (typeof v === "string") {
    if (!datePattern.test(v)) {
      return false;
    }
  }

  return origIsDateString.apply(m, [v]);
};

@sgrebnov
Copy link

Looks like parameter should be escaped so it works correctly (string is passed as-is w/o attempt to be converted to date time); The following works for me:
?filter=someParam||cont||CG-7 -> ?filter=someParam||cont||"CG-7"

@Bnaya
Copy link
Author

Bnaya commented Jul 21, 2019

I think its due to json.parse that is part of the pipeline
This behavior is not documented and i dont see why regular string wont work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants