Skip to content

feat: hooks round 2 - remove indirection and simplify error handling #2025

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 19, 2025
Merged

feat: hooks round 2 - remove indirection and simplify error handling #2025

merged 7 commits into from
May 19, 2025

Conversation

cstockton
Copy link
Contributor

@cstockton cstockton commented May 15, 2025 •
edited
Loading

Hooks Round 2

This PR will contain a series of commits preparing for the implementation of before & after user created hooks. It takes the feedback from #2012 into consideration.

Summary

Remove indirection and simplify error handling:

  • update pkg internal/api to:
    • uses internal/hooks/v0hooks.Manager instead of internal/hooks/hooks.Manager aec5995
  • remove pkg internal/hooks/hooks.Manager 062da5d
  • add pkg internal/hooks/hookserrors 7e80afc
  • use pkg internal/hooks/hookserrors in internal/hooks/v0hooks 57744e8
  • update pkg internal/hooks/v0hooks with an Enabled method 16cc4c9

Depends on

feat: hooks round 1 - prepare package structure

  • Renamed pkg internal/hooks/v0hooks/v0http -> internal/hooks/hookshttp 8a398ab
  • Renamed pkg internal/hooks/v0hooks/v0pgfunc -> internal/hooks/hookspgfunc 8a398ab
  • Use pkg internal/e2e for test setup in:
    • pkg internal/hooks/hookspgfunc 4d60288
    • pkg internal/hooks/v0hooks 4a7432b

@cstockton cstockton requested a review from a team as a code owner May 15, 2025 15:42
@coveralls
Copy link

coveralls commented May 15, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 15118259181

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 319 of 321 (99.38%) changed or added relevant lines in 11 files are covered.
  • 13 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.4%) to 70.016%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/token.go 17 19 89.47%
Files with Coverage Reduction New Missed Lines %
internal/api/api.go 2 84.55%
internal/api/token_refresh.go 2 67.69%
internal/api/web3.go 9 88.98%
Totals Coverage Status
Change from base Build 15049009181: 0.4%
Covered Lines: 11129
Relevant Lines: 15895
💛 - Coveralls

Chris Stockton added 4 commits May 15, 2025 09:29
feat: add pkg hookserrors for consistent error handling
7e80afc 
Package hookserrors holds the Error type and some functions to
Check responses for errors.
feat: use pkg hookserrors in hookshttp & hookspgfunc
57744e8 
This allows consistent error handling across both the hookshttp
and hookspgfunc packages without the need to embed AuthHookError
in every single output struct.

In addition this required moving `validateTokenClaims` to the
API package. This removes the need for a single special case
in the dispatching of hooks in pkg `internal/hooks/v0hooks`.
feat: add Enabled method to internal/hooks/v0hooks.Manager
16cc4c9 
This creates a concrete mapping of const -> config value. This
will be used in future pull requests as well as the new Enabled
method which guards calls to disabled hooks in v0hooks.Manager.
fix: remove two const to satisfy the exhaustive check
d758acc
@cstockton cstockton changed the title chore: use v0hooks.Manager directly in api pkg feat: hooks round 2 - remove indirection and simplify error handling May 16, 2025
@cstockton cstockton mentioned this pull request May 16, 2025
Merged
hf
hf approved these changes May 16, 2025
View reviewed changes
Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

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

LGTM

@cstockton
feat: hooks round 3 - begin adding the Before and After user created …
d09d08d 
…hooks (#2028)

## Hooks Round 3

This PR will contain a series of commits preparing for the
implementation of before & after user created hooks. It takes the
feedback from #2012 into
consideration.

### Summary

Begin adding the Before and After user created hooks:
* update pkg `internal/conf`
[d5f5436](d5f5436)
* add `BeforeUserCreated` and `AfterUserCreated` to `HookConfiguration`
struct
* add test cases for `EmailValidationBlockedMX` to restore 100% test
coverage
* update pkg `internal/hooks/v0hooks`
[bd37fe2](bd37fe2)
    * add `BeforeUserCreated` method to `v0hooks.Manager` struct
    * add `AfterUserCreated` method to `v0hooks.Manager` struct
    * add tests to reach 100% coverage
* add pkg `internal/e2e/e2ehooks`
[903e623](903e623)
    * add `HookCall` to record calls to hooks
    * add `Hook` struct to hold `[]*HookCall` for a given hook name
    * add `HookRecorder` to hold one `Hook` object per hook name
* add `Instance` struct to hold the `httptest.Server` and `HookRecorder`
    * add `AfterUserCreated` method to `v0hooks.Manager` struct
    * add tests to reach 100% coverage in all `internal/e2e` packages
* update pkg `internal/hooks/v0hooks`
[829aec6](829aec6)
    * fix struct and json tag to match to match the Metadata type
* update pkg `internal/hooks/v0hooks`
[ca67be0](ca67be0)
    * remove `BeforeUserCreated` and `AfterUserCreated` methods
    * add Before & After user created hooks in `InvokeHook`
* update pkg `internal/e2e/e2eapi`
[46c144e](46c144e)
    * add comments in IOError tests involving `http.RoundTripper`
    * update calls to `t.Fatal` to use `require`

### Depends on
[feat: hooks round 1](#2023) -
prepare package structure
* Renamed pkg `internal/hooks/v0hooks/v0http` ->
`internal/hooks/hookshttp`
[8a398ab](8a398ab)
* Renamed pkg `internal/hooks/v0hooks/v0pgfunc` ->
`internal/hooks/hookspgfunc`
[8a398ab](8a398ab)
* Use pkg `internal/e2e` for test setup in:
* pkg `internal/hooks/hookspgfunc`
[4d60288](4d60288)
* pkg `internal/hooks/v0hooks`
[4a7432b](4a7432b)

[feat: hooks round 2](#2025) -
remove indirection and simplify error handling
* update pkg `internal/api` to:
* uses `internal/hooks/v0hooks.Manager` instead of
`internal/hooks/hooks.Manager`
[aec5995](aec5995)
* remove pkg `internal/hooks/hooks.Manager`
[062da5d](062da5d)
* add pkg `internal/hooks/hookserrors`
[7e80afc](7e80afc)
* use pkg `internal/hooks/hookserrors` in `internal/hooks/v0hooks`
[57744e8](57744e8)
* update pkg `internal/hooks/v0hooks` with an `Enabled` method
[16cc4c9](16cc4c9)

---------

Co-authored-by: Chris Stockton <[email protected]>
@cstockton cstockton merged commit 26e23f0 into master May 19, 2025
5 checks passed
@cstockton cstockton deleted the cs/hooks-p2 branch May 19, 2025 16:37
@github-actions github-actions bot mentioned this pull request May 19, 2025
Open
@cstockton cstockton restored the cs/hooks-p2 branch May 19, 2025 19:07
@cstockton cstockton mentioned this pull request May 20, 2025
Merged
cstockton added a commit that referenced this pull request May 20, 2025
@cstockton
feat: hooks round 4 - update tests to use require package (#2030)
aaf93df 
## Hooks Round 4

This PR will contain a series of commits preparing for the
implementation of before & after user created hooks. It takes the
feedback from #2012 into
consideration.

### Summary

[feat: hooks round 4](#2030) -
update tests to use require package
* use pkg `require` for tests in:
[f2b3600](f2b3600)
    * pkg `internal/e2e/...`
    * pkg `internal/hooks/...`


### Depends on

[feat: hooks round 1](#2023) -
prepare package structure
* Renamed pkg `internal/hooks/v0hooks/v0http` ->
`internal/hooks/hookshttp`
[8a398ab](8a398ab)
* Renamed pkg `internal/hooks/v0hooks/v0pgfunc` ->
`internal/hooks/hookspgfunc`
[8a398ab](8a398ab)
* Use pkg `internal/e2e` for test setup in:
* pkg `internal/hooks/hookspgfunc`
[4d60288](4d60288)
* pkg `internal/hooks/v0hooks`
[4a7432b](4a7432b)

[feat: hooks round 2](#2025) -
remove indirection and simplify error handling
* update pkg `internal/api` to:
* uses `internal/hooks/v0hooks.Manager` instead of
`internal/hooks/hooks.Manager`
[aec5995](aec5995)
* remove pkg `internal/hooks/hooks.Manager`
[062da5d](062da5d)
* add pkg `internal/hooks/hookserrors`
[7e80afc](7e80afc)
* use pkg `internal/hooks/hookserrors` in `internal/hooks/v0hooks`
[57744e8](57744e8)
* update pkg `internal/hooks/v0hooks` with an `Enabled` method
[16cc4c9](16cc4c9)

[feat: hooks round 3](#2028) -
begin adding the Before and After user created hooks
* update pkg `internal/conf`
[d5f5436](d5f5436)
* add `BeforeUserCreated` and `AfterUserCreated` to `HookConfiguration`
struct
* add test cases for `EmailValidationBlockedMX` to restore 100% test
coverage
* update pkg `internal/hooks/v0hooks`
[bd37fe2](bd37fe2)
    * add `BeforeUserCreated` method to `v0hooks.Manager` struct
    * add `AfterUserCreated` method to `v0hooks.Manager` struct
    * add tests to reach 100% coverage
* add pkg `internal/e2e/e2ehooks`
[903e623](903e623)
    * add `HookCall` to record calls to hooks
    * add `Hook` struct to hold `[]*HookCall` for a given hook name
    * add `HookRecorder` to hold one `Hook` object per hook name
* add `Instance` struct to hold the `httptest.Server` and `HookRecorder`
    * add `AfterUserCreated` method to `v0hooks.Manager` struct
    * add tests to reach 100% coverage in all `internal/e2e` packages
* update pkg `internal/hooks/v0hooks`
[829aec6](829aec6)
    * fix struct and json tag to match to match the Metadata type
* update pkg `internal/hooks/v0hooks`
[ca67be0](ca67be0)
    * remove `BeforeUserCreated` and `AfterUserCreated` methods
    * add Before & After user created hooks in `InvokeHook`
* update pkg `internal/e2e/e2eapi`
[46c144e](46c144e)
    * add comments in IOError tests involving `http.RoundTripper`
    * update calls to `t.Fatal` to use `require`

Co-authored-by: Chris Stockton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hf hf hf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cstockton @coveralls @hf