In this post I'll be reviewing my progress in open source over the past few weeks.
Originally, my plan for this past month or so was to work on Mattermost Mobile, where I was going to add a feature to toggle rendering emoticons as emojis. I worked on the same feature in the webapp, so I figured I should have a solid idea of what I needed to do. Quick update on the webapp pull request by the way, my changes were finally reviewed by the contributors. I'll be working on making the requested changes over the next few days.
Anyway, unfortunately, I ended up being way busier than I could've anticipated and I wasn't able to start on the mobile issue until very recently, where I immediately hit a roadblock in not being able to set up the dev environment due to my operating system, meaning I couldn't do the work I was planning on, and had to change gears fast to still get contributions in. I decided my best option was to look for smaller, but still substantial issues.
I looked through GitHub for issues labelled "help wanted", and ended up finding a couple more Mattermost issues, along with one for the GitHub CLI:
Issue 1: Github CLI - Improving output formatting
`gh run view` should list branches in square brackets
#10038
gh run view lists branches in parenthesis, but I think it should list them in square brackets to align with Primer guidelines:
Display branch names in brackets and/or cyan
gh run view
gh run view prompts should display branches within square brackets.
❯ gh run view
? Select a workflow run [Use arrows to move, type to filter]
> - Verifying attestations offline fails, Discussion Triage (trunk) 4h55m1s ago
- Decoding, Discussion Triage (patch-1) 4h59m32s ago
✓ Decoding, PR Automation (patch-1) 4h59m43s ago
✓ Issue Automation, Issue Automation (trunk) 5h20m31s ago
- `gh repo rename myorg/newname` results in `myorg/myorg-newname`, Discussion Triage (trunk) 10h13m50s ago
- 401 Error at every turn, Discussion Triage (trunk) 10h15m20s ago
- 401 Error at every turn, Discussion Triage (trunk) 10h15m20s ago
Issue 2: Mattermost - Adding timestamps to pasted image filenames
Add timestamp to pasted image filenames to differentiate them
#29524
When pasting an image from the clipboard into the message input, it gets the filename image.png. If multiple images are pasted, they all get the same image.png filename. This can be a problem when downloading each file with the same name with danger of overwriting.
Auto-generate a Filename that includes the timestamp with this format:
image-2024-11-18-6-29-57-PM
Mattermost thread: https://hub.mattermost.com/private-core/pl/xryg3tedg3bbxq3xuwnrj9ymyc
If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.
New contributors please see our Developer's Guide.
Issue 3: Mattermost - Improving SQL query maintainability and backward compatibility
Avoid SELECT * in tokens_store.go
#29548
SELECT * makes ADD COLUMN a backwards incompatible schema change: older servers running against a newer schema won’t know what to do with the new columns.
This ticket tracks the effort required to migrate the SQL queries in https://github.com/mattermost/mattermost/blob/master/server/channels/store/sqlstore/tokens_store.go . (Please submit a single PR with changes to only this store.)
Step 1: Migrate any SELECT * queries written as strings into builder queries.
First, if the query is hard-coded as a string, e.g. SELECT * FROM Table WHERE ..., migrate this query to using the SQLBuilder. So instead of this:
if _, err = s.GetReplica().Get("SELECT * FROM Table WHERE Id = ?", id); err != nil {
return err
}
change it to this instead:
query := s.getQueryBuilder().
Select("*").
From("Table").
Where(sq.Eq{"Id": id})
if _, err = s.GetReplica().GetBuilder(query); err != nil {
return err
}
It’s not necessary to change all queries in this [file: jus|file: ju]t the ones doing a SELECT *.
Step 2: Migrate any instances of SELECT * into explicitly enumerating the needed columns
Instead of SELECT("*"), rewrite to explicitly specify the columns needed:
query := s.getQueryBuilder().
Select("Column1", "Column2").
From("Table").
Where(sq.Eq{"Id": id})
If these columns are referenced more than once in the file, factor out the query into the store constructor:
type SqlTableStore struct {
*SqlStore
tableSelectQuery sq.SelectBuilder
}
func newSqlTableStore(sqlStore *SqlStore) store.TableStore {
s := SqlTableStore{
SqlStore: sqlStore,
}
s.tableSelectQuery = s.getQueryBuilder().
Select("Column1", "Column2").
From("Table")
return &s
}
and then extend it everywhere:
query := s.tableSelectQuery.Where(sq.Eq{"Id": id})
Step 3: Run the related unit tests
Within the server/channels/store/sqlstore run the tests for the associated store, e.g. go test -run TestTableStore (replace with the test for the store in question).
If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.
New contributors please see our Developer's Guide.
How It Went
I'll talk about the issues in order.
Issue 1: Github CLI - Improving output formatting
This issue was a much needed change of pace from the gargantuan issue I was working on for Mattermost. After struggling with the dev environment setup for days and failing, I needed something to get back into the flow of things and this issue was perfect.
The maintainers wanted to improve the output formatting for the gh run view subcommand to bring it closer in line with GitHub's design guidelines by displaying branch names in square brackets instead of parentheses.
Setting up the dev environment was super simple because the only thing I needed to install was Go. I started, as always, by looking at the contributing docs, where I found a nice explanation of what goes in each project directory, which helped me find the file containing the subcommand's logic. I noticed the display function wasn't actually in that file and traced it back to a common utilities file for the run command. From there, I just updated the display function to print square brackets instead of parentheses. I noticed it looked very reminiscent of C's printf, which is apparently because Go is designed after C.
Testing my changes manually was also very straightforward. It was a single command to build the binary, and then I could use it from the terminal. So I made the change and submitted my PR but forgot to run and update unit tests which I then corrected as soon as I realized.
At this point the maintainer who opened the issue informed me that the issue hadn't been triaged yet and they weren't sure if they wanted to actually implement this. Fortunately, they later did agree on the changes, only with the additional acceptance criteria of also updating the formatting for the gh codespace command.
So I made the changes - again, pretty straightforward process (Didn't forget to update the tests this time), and my changes were approved and merged. All in all, a pretty relaxed experience compared to other stuff I've worked on.
Issue 2: Mattermost - Adding timestamps to pasted image filenames
For this issue, the maintainers noticed that images pasted from the clipboard would all receive the name "image.png", and wanted to add timestamps on paste, to help differentiate them in case they overwrite each other when downloading them.
This seemed like it'd be a pretty straightforward issue, but actually ended up being pretty interesting, although me and the maintainers couldn't come to an agreement on the solution.
The issue stemmed from the browser automatically assigning the file name "image.png" to clipboard images. Now, simply adding a check for "image.png" would be stupid simple, but of course it's never that easy. I found this thread on the Atlassian Confluence forums about a very similar issue. Confluence appended timestamps to images with a hardcoded check for the filename "image.png", but things broke when Firefox started localizing the filename, leading to German users ending up with the filename "grafik.png".
Even though I manually tested the localization by switching Firefox to German and wasn't able to reproduce it (i.e. it remained "image.png"), the possibility that the filename could change at all on different platforms/environments meant I couldn't just hardcode the check.
Another problem was that hardcoding the check meant if a user actually had a file named "image.png" we would end up modifying the filename which we didn't want to do. So, we needed a way to differentiate user-defined filenames and automatically generated filenames, without hardcoding them.
Unfortunately, all we had to work with was a ClipboardEvent, which didn't give us enough information to tell the difference. So it seems like the desired solution, where we wouldn't modify filenames unless they were automatically generated was not possible.
I proposed an alternative where we could append timestamps to all files, either on upload on download, maybe as a toggle-able setting, but this doesn't really directly address the issue. I've yet to hear back on what direction the maintainers want to go in, if they still want to implement this at all.
Issue 3: Mattermost - Improving SQL query maintainability and backward compatibility
This issue stands out a bit among other issues I've worked on so far because it finally involves updating backend logic. The idea is to improve database queries by switching from SELECT * to explicitly defining columns, ensuring backwards compatibility with database schema changes (older servers running against a newer schema won’t know what to do with the new columns). In addition, to improve maintainability, the queries were to be migrated from hardcoded strings to being constructed using an SQL query builder API.
When I started working on this issue, I noticed the issue description talked about using a method called getReplica() but that method didn't seem to exist, so I referenced an existing function in the same file that already used the query builder and used getReplicaX().
In order to find out what columns the table actually contained, I needed to go into the database and look around, but I noticed there were two database containers being spun up - MySQL and Postgres. I looked at the docs and saw that Postgres was preferred for new deployments and that there were in-app settings to choose the database, so I went into the settings to take a look and confirmed that Postgres was in fact selected, so I figured I should probably check out the Postgres container. I needed the database username so I went into the Docker Compose file and found an environment variable for it. I then used docker exec to run psql and find the table and it's columns.
In order to figure out query builder syntax for selecting multiple columns, I searched the repo for the string getQueryBuilder( and found an example at server/channels/store/sqlstore/bot_store.go.
I also factored out the common query (SELECT token, createat, type, extra FROM Tokens) into the type and it's constructor like the issue said and modified the other queries to use the newly factored-out query.
So I made the pull request, but I realized the maintainer ended up updating the file I was working on after I'd made my branch, and this led to merge conflicts. Following their contribution guidelines, instead of rebasing I merged master into my branch. Remember how I couldn't find the getReplica() method? They'd updated the file to now use that method. This method had different syntax so I had to go and look for examples so I could update my changes while resolving the merge conflict.
This PR is still pending review.
Final Thoughts
In this process, I was exposed to Go, psql, and Web APIs like ClipboardEvent and DataTransferItem, which made for a fun learning experience. In fact, I ended up working with Go in two out of three of the issues which I thought was interesting. Seems like Go is growing in popularity.
That's about it for this post. This also marks the end of the course I'm taking on Open Source Development, OSD600 at Seneca Polytechnic. It's been a fantastic journey starting from barely understanding how to use Git to being able to use it to effectively contribute to huge projects. The course may be over but I'm certain I'll continue to work in open source in the future. Thanks for reading.
Top comments (0)