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: replace message-related field into JSON type from JSONB #114

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Mar 20, 2024

Description

This PR intends to fix the issue that message JSON may includes invalid Unicode sequence \u0000, it would cause the error when saving transactions and messages into Database.

Reference:
Invalid unicode sequence on PostgreSQL
Jackal transaction including invalid unicode sequence

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Re-reviewed Files changed in the Github PR explorer.

@dadamu dadamu changed the title fix: replace message related to json into JSON type from JSONB fix: replace message-related field into JSON type from JSONB Mar 20, 2024
Copy link

@0x7u 0x7u left a comment

Choose a reason for hiding this comment

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

Good catch 👍🏻 . It's also a good learning.

These are the trade-offs using JSON vs JSONB type:

The json and jsonb data types accept almost identical sets of values as input. The major practical difference is one of efficiency. The json data type stores an exact copy of the input text, which processing functions must reparse on each execution; while jsonb data is stored in a decomposed binary format that makes it slightly slower to input due to added conversion overhead, but significantly faster to process, since no reparsing is needed. jsonb also supports indexing, which can be a significant advantage.

https://www.postgresql.org/docs/current/datatype-json.html#:~:text=The%20json%20data%20type%20stores,since%20no%20reparsing%20is%20needed.

We are not indexing this column? (I believe we are not, just want to make sure)

@dadamu
Copy link
Contributor Author

dadamu commented Mar 21, 2024

@0x7u We didn't index on JSONB column, to make sure the data is exactly the same as blockchain shows, we should allow \u0000 input used by chains, especially Jackal.

About trading-off the performance, the document said JSONB has better performance on processing, but we don't use any function and indexing to process on it, therefore it is safe to migrate JSONB to JSON.

dadamu added a commit to forbole/callisto that referenced this pull request Apr 17, 2024
## Description

This PR intends to fix the issue that message JSON may includes invalid
Unicode sequence \u0000, it would cause the error when saving
transactions and messages into Database.

Reference:
Invalid unicode sequence on PostgreSQL
Jackal transaction including invalid unicode sequence

Related issue: forbole/juno#114

<!-- Add a description of the changes that this PR introduces and the
files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is
not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type
prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json)
in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch
- [ ] provided a link to the relevant issue or specification
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go
code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable
and please add
your handle next to the items reviewed if you only reviewed selected
items.*

I have...

- [ ] confirmed the correct [type
prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json)
in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@dadamu dadamu merged commit f839638 into cosmos/v0.47.x Apr 17, 2024
4 checks passed
@dadamu dadamu deleted the paul/replace-message-related-json-into-json branch April 17, 2024 14:58
dzmitryhil pushed a commit to CoreumFoundation/junov5 that referenced this pull request Dec 16, 2024
…#114)

<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰    
v    Before smashing the submit button please review the checkboxes.
v If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description
<!-- Small description -->
This PR intends to fix the issue that message JSON may includes invalid
Unicode sequence \u0000, it would cause the error when saving
transactions and messages into Database.

Reference:
[Invalid unicode sequence on
PostgreSQL](https://stackoverflow.com/questions/68303357/how-to-insert-json-containing-escape-codes-into-a-jsonb-column-in-postgresql-usi)
[Jackal transaction including invalid unicode
sequence](https://api.jackal.forbole.com/cosmos/tx/v1beta1/txs/0C51E15736E66F17D26E2FB93E3591E81E9C0D3F7477D9F0DA383B037CB23CC1)

## Checklist
- [ ] Targeted PR against correct branch.
- [ ] Linked to Github issue with discussion and accepted design OR link
to spec that describes this work.
- [ ] Wrote unit tests.  
- [ ] Re-reviewed `Files changed` in the Github PR explorer.
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

Successfully merging this pull request may close these issues.

2 participants