Skip to content

Conversation

@jsco2t
Copy link
Contributor

@jsco2t jsco2t commented Nov 6, 2025

This is a wrap-up PR for the work that NH started.

The original issue is here:

https://github.com/rocky-linux/mattermost-plugin-community-toolkit/issues/6

The original PR from NH is here:

https://github.com/rocky-linux/mattermost-plugin-community-toolkit/pull/7

To summarize all of the new changes in this branch:

  • Added the ability to deploy a local-dev mattermost instance to test the plugin.
  • Resolved some bugs related to domain and user name blocking
  • Added in more test automation + tests in depth for new features
  • Documentation cleanup
  • Fixed issues reported by golang linting tools
  • Tuned LRUCache for slight performance improvemen

NeilHanlon and others added 6 commits April 4, 2025 10:15
Resolves conflicts by keeping test coverage from both branches:
- Indefinite duration tests from feature branch
- Comprehensive test suite and ExtendedMockAPI from main
… moderation

This is a wrap-up PR for the work that NH started.

The original issue is here:

rocky-linux#6

The original PR from NH is here:

rocky-linux#7

To summarize all of the *new* changes in this branch:
- Added the ability to deploy a local-dev mattermost instance to test
the plugin.
- Resolved some bugs related to domain and user name blocking
- Added in more test automation + tests in depth for new features
- Documentation cleanup
- Fixed issues reported by golang linting tools
- Tuned LRUCache for slight performance improvement
@NeilHanlon
Copy link
Member

Thanks very much for your work on this and your patience in awaiting a review. I began reviewing it but am having trouble getting a good grasp on all the changes because there's a lot of unrelated work bundled together. With a 10k line delta across 28 files it's difficult to give each part the attention it deserves.

Looking at the bullet points in the PR, could you break this up into separate PRs? Something like:

  1. Local dev environment (podman/makefile/scripts)
  2. Test infrastructure improvements
  3. Domain/username validation features
  4. Content blocking features (links/images/dms)
  5. User cleanup/enforcement logic
  6. Documentation updates

This would make it much easier to review each piece properly and understand the interactions between changes. It would also make rollback & bisection simpler if any issues come up later.

I did spot one issue (I think) in the LRU cache implementation: in Get(), there's a small gap between releasing the read lock and acquiring the write lock where another goroutine can evict or remove the entry. Because the previously-fetched pointer is still used after reacquiring the write lock, Get() can return a "hit" for a key that is no longer present in the map. It’s not a data race, but it does break expected LRU semantics.

A straightforward fix would be to re-check the entry once the write lock is held (or just hold the write lock for the entire Get() path). Either approach removes the TOCTOU window.

Happy to dig into this (and everything else) once the PR is split up.

@jsco2t
Copy link
Contributor Author

jsco2t commented Nov 21, 2025

Thanks for the feedback @NeilHanlon - I'll work on breaking this up.

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