Skip to content

feat(libs/pubsub): allow dash symbol in event type/attribute#3456

Merged
melekes merged 5 commits intomainfrom
3401-dash-event
Jul 9, 2024
Merged

feat(libs/pubsub): allow dash symbol in event type/attribute#3456
melekes merged 5 commits intomainfrom
3401-dash-event

Conversation

@melekes
Copy link
Collaborator

@melekes melekes commented Jul 8, 2024

Closes #3401

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@melekes melekes requested a review from a team as a code owner July 8, 2024 06:52
@melekes melekes requested a review from a team July 8, 2024 06:52
@melekes melekes mentioned this pull request Jul 8, 2024
Closed
4 tasks
@melekes melekes self-assigned this Jul 8, 2024
@melekes melekes added backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x and removed backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x labels Jul 8, 2024
@melekes melekes enabled auto-merge July 8, 2024 06:58
@melekes melekes disabled auto-merge July 8, 2024 07:21
@melekes melekes added the indexer label Jul 8, 2024
@melekes
Copy link
Collaborator Author

melekes commented Jul 8, 2024

cc @Lockwarr

cason
cason approved these changes Jul 8, 2024
View reviewed changes
Copy link

@cason cason left a comment

Choose a reason for hiding this comment

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

Looks neat, thank you for updating also the documentation.

@Lockwarr
Copy link
Contributor

Lockwarr commented Jul 8, 2024

Just to clarify, is this going to be backported into the v0.38.x line?

@melekes
Copy link
Collaborator Author

melekes commented Jul 9, 2024

Just to clarify, is this going to be backported into the v0.38.x line?

We are not going to backport this since we consider this a feature, not a bug. We never claimed to support the dash symbol, only [a-zA-Z_.]. For instance, if tomorrow someone comes and tells us they want keys to contain the $ sign, we are going to treat it as a feature too. According to our backporting policy, v0.38 and v0.37 will only backport bug fixes.

@melekes melekes added this pull request to the merge queue Jul 9, 2024
Merged via the queue into main with commit a3ec97e Jul 9, 2024
@melekes melekes deleted the 3401-dash-event branch July 9, 2024 06:39
@Lockwarr
Copy link
Contributor

Lockwarr commented Jul 9, 2024

Well, in the v0.37 line, querying wasm- events worked fine. I saw there was some sort of refactoring of the way queries are scanned.
if tomorrow someone comes and tells us they want keys to contain the $ sign I disagree with this. I am not trying to get a feature for a new symbol approved. I'm simply migrating our l1 and wanted to keep the functionality from previous versions. If this is not backported in v0.38, we will be blocked until the new release(v1?) as querying events is crucial part for debugging
cc @melekes @hvanz

@melekes
Copy link
Collaborator Author

melekes commented Jul 9, 2024

Thanks for clarifying that @Lockwarr. So it appears we were accidentally supporting - symbol (and probably many others) due to the bug? in v0.37 query parser's implementation. I think we can backport this PR, but I'm a little bit afraid of setting a bad precedent (i.e. if - worked in v0.37 and we fixed that, why not allow $, (, ) and all other existing symbols). From the implementation perspective, we actually don't care if a key is $%^&**), but we were trying to keep things as simple as possible.

@melekes
Copy link
Collaborator Author

melekes commented Jul 9, 2024

@mergify backport v0.38.x

@melekes
Copy link
Collaborator Author

melekes commented Jul 9, 2024

@mergify backport v1.x

@mergify
Copy link
Contributor

mergify bot commented Jul 9, 2024 •
edited
Loading

backport v0.38.x

✅ Backports have been created

Details

@mergify
Copy link
Contributor

mergify bot commented Jul 9, 2024 •
edited
Loading

backport v1.x

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Jul 9, 2024
@melekes @mergify
feat(libs/pubsub): allow dash symbol in event type/attribute (#3456)
e465cab 
Closes #3401

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Lockwarr <denislav66@gmail.com>
(cherry picked from commit a3ec97e)

# Conflicts:
#	docs/app-dev/indexing-transactions.md
mergify bot pushed a commit that referenced this pull request Jul 9, 2024
@melekes @mergify
feat(libs/pubsub): allow dash symbol in event type/attribute (#3456)
2ccba79 
Closes #3401

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Lockwarr <denislav66@gmail.com>
(cherry picked from commit a3ec97e)

# Conflicts:
#	docs/guides/app-dev/indexing-transactions.md
This was referenced Jul 9, 2024
Merged
Merged
melekes added a commit that referenced this pull request Jul 9, 2024
@mergify @melekes
feat(libs/pubsub): allow dash symbol in event type/attribute (backport …
a1b6c68 
…#3456) (#3461)

Closes #3401 

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3456 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
melekes added a commit that referenced this pull request Jul 9, 2024
@mergify @melekes
feat(libs/pubsub): allow dash symbol in event type/attribute (backport …
a095cb8 
…#3456) (#3462)

Closes #3401 

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3456 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@faddat faddat mentioned this pull request Oct 15, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@cason cason cason approved these changes

@hvanz hvanz hvanz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@melekes melekes

Labels

indexer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix querying events with dash(-) in their name

4 participants

@melekes @Lockwarr @cason @hvanz