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

bindParam missing from AbstractDataType type definition #11550

Closed
defunctzombie opened this issue Oct 14, 2019 · 7 comments · Fixed by #14505
Closed

bindParam missing from AbstractDataType type definition #11550

defunctzombie opened this issue Oct 14, 2019 · 7 comments · Fixed by #14505
Labels
status: understood For issues. Applied when the issue is understood / reproducible. type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.

Comments

@defunctzombie
Copy link
Contributor

The bindParam function is missing from the AbstractDataTypes type definition. The result is that a user who implements a custom type must be aware to also implement the bindParam function if they want to do custom escaping or formatting of their type when used with the bind query interface.

Previously it was enough to just support stringify but this is no longer the case with the new bind based query builder.

@sushantdhiman sushantdhiman added the type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Oct 16, 2019
@papb
Copy link
Member

papb commented Oct 30, 2019

Hello, can you please show some code that should compile but doesn't? Thanks

@papb papb added the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Oct 30, 2019
@defunctzombie
Copy link
Contributor Author

Absolutely. Tho the issue isn't that the code doesn't compile - its that without the bindParam being a required method to define my custom type did not work properly.

Below is my custom type definition which is meant to be a wrapper around the postgres NUMERIC type using the big.js library to support arbitrary precision and accurate decimal math operations in my application logic.

import { AbstractDataType } from 'sequelize';
import Big from 'big.js';

export default class BigType implements AbstractDataType {
  public dialectTypes: string = '';
  public key: string = 'NUMERIC';

  toSql(): string {
    return 'NUMERIC';
  }

  toString(): string {
    return 'NUMERIC';
  }

  stringify(value: Big): string {
    return value.toString();
  }

  bindParam(value: Big): string {
    return value.toString();
  }

  _sanitize(value: string): Big {
    return Big(value);
  }

  parse(value: string): Big {
    return Big(value);
  }
}

When I ported from v4 to v5 I noticed that my queries were failing. I traced the problem down to missing the bindParam method from my implementation. Without this method present in the class, the queries were not serializing properly - this was a runtime error rather than compile time error.

After debugging by digging into the query generator code, I realized that I needed to add the bindParam method but this method was not part of AbstractDataType and so was not obvious to me that I needed to include it in my type for it to properly serialize. If the bindParam method was in the interface I would have gotten a build time error (what I wanted to see) and it would be obvious that I am missing an important method for proper functioning of my custom type.

@papb
Copy link
Member

papb commented Jan 15, 2020

Ah, interesting! Thanks for the explanation and sorry to take so long to reply. It looks like fixing this is quite simple then, just adding one line to the type declarations, am I right? Or did I misunderstand something?

Also, by the way, since I noticed you are implementing a custom data type, is there any chance you could take a look at this issue to see if you have any ideas? #11177 Many people is currently having trouble to make a custom data type work - myself included. The steps provided in our current docs don't work. Thank you!!

@papb papb mentioned this issue Jan 15, 2020
7 tasks
@defunctzombie
Copy link
Contributor Author

just adding one line to the type declarations, am I right

Yep. I believe that would do it.

Also, by the way, since I noticed you are implementing a custom data type, is there any chance you could take a look at this issue to see if you have any ideas? #11177 Many people is currently having trouble to make a custom data type work - myself included.

I took a quick look at the issue and my initial impression is maybe the missing bindParam implementation. The referenced issue starts with someone wanting to implement a custom big number datatype - which is exactly what I needed. The code I pasted above should work for that use case (it does for me) granted I am using postgres rather than mysql as the original issue states.

@papb
Copy link
Member

papb commented Jan 19, 2020

@defunctzombie Thank you!! I will look into it

@papb papb self-assigned this Jan 19, 2020
@papb papb added status: understood For issues. Applied when the issue is understood / reproducible. and removed status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action labels Jan 19, 2020
@defunctzombie
Copy link
Contributor Author

Is this issue still awaiting any other feedback?

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2021

This issue has been automatically marked as stale because it has been open for 7 days without activity. It will be closed if no further activity occurs. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

@github-actions github-actions bot added the stale label Nov 6, 2021
@WikiRik WikiRik removed the stale label Nov 15, 2021
@ephys ephys mentioned this issue Oct 6, 2022
42 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: understood For issues. Applied when the issue is understood / reproducible. type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants