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

fix: Parsing for qualifiers with colon characters #1277

Merged
merged 13 commits into from
May 24, 2023

Conversation

KDB223
Copy link
Contributor

@KDB223 KDB223 commented May 12, 2023

parseColumnName() uses .split(':') to find the qualifier, but this parses qualifiers with : chars in it incorrectly, since they also are picked up by the split. This causes it to ignore everything after : in the qualifier.

e.g. consider a column name "foo:bar:baz"
parseColumnName() parses this incorrectly as

{
  family: 'foo', 
  qualifier: 'bar'
}

Using slice() instead with the index of the first : character fixes this:

{
  family: 'foo', 
  qualifier: 'bar:baz'
}
@KDB223 KDB223 requested review from a team as code owners May 12, 2023 09:54
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels May 12, 2023
@KDB223 KDB223 changed the title Fix parsing for qualifiers with colon characters May 12, 2023
@conventional-commit-lint-gcf
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@danieljbruce
Copy link
Contributor

Please fix the tests should array-ify the input and should parse a family name and modify the last commit so that the conventional commits github action passes.

@generated-files-bot
Copy link

Warning: This pull request is touching the following templated files:

  • .github/workflows/ci.yaml - .github/workflows/ci.yaml (GitHub Actions) should be updated in synthtool
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels May 12, 2023
@KDB223
Copy link
Contributor Author

KDB223 commented May 12, 2023

All done, tested locally. Apologies for the noise.

Changed the qualifier assertion to check for null instead of undefined since we can't return undefined from the method.

src/mutation.ts Outdated
// columnName does not contain ':'
return {
family: columnName,
qualifier: null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend setting qualifier to undefined so that this code works the same way as before when the string doesn't contain a colon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that too, however the ParsedColumn interface does not declare undefined as an acceptable type for qualifier at https://github.com/googleapis/nodejs-bigtable/blob/main/src/mutation.ts#L35.

Adding undefined here would solve this:

export interface ParsedColumn {
  family: string | null;
  qualifier: string | null | undefined;
}
@danieljbruce
Copy link
Contributor

Consider adding another test?

@bcoe bcoe added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels May 24, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 24, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2023
@danieljbruce danieljbruce added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 24, 2023
@danieljbruce danieljbruce merged commit b80f533 into googleapis:main May 24, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the googleapis/nodejs-bigtable API. kokoro:run Add this label to force Kokoro to re-run the tests. size: s Pull request size is small.
4 participants