Skip to content

Commit 219b15e

Browse files
authored
fix(public-api/v1alpha): improve integer parsing in gofer request formatter (#672)
## 📝 Description Fix parsing of `page/page_size` in the promotions listing request so numeric strings (like `page=2`) pass through instead of triggering 400s. Check [the issue](renderedtext/tasks#8815). ## ✅ Checklist - [x] I have tested this change - [x] ~This change requires documentation update~ N/A
1 parent 7e7ba78 commit 219b15e

File tree

8 files changed

+125
-5
lines changed

8 files changed

+125
-5
lines changed

public-api/v1alpha/AGENTS.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# Repository Guidelines
2+
3+
## Project Structure & Module Organization
4+
Source lives in `lib/pipelines_api`, which exposes HTTP controllers that call internal gRPC clients. Regenerated gRPC stubs sit in `lib/internal_api`; rerun `make pb.gen` instead of editing them. Environment configuration is under `config/*.exs` (overrides in `config/runtime.exs`). Shared scripts live in `priv/script`, Helm assets in `helm/`, and ExUnit fixtures in `test/` plus `test/support`.
5+
6+
## Build, Test, and Development Commands
7+
Use `make console` to enter the Docker dev container with dependencies mounted. Start the server via `iex -S mix` inside that shell; it binds to port 4004 per `docker-compose.yml`. Run the suite through `make unit-test` or `mix test`. Refresh dependencies with `mix deps.get`, and enforce formatting with `mix format`. Static analysis helpers include `mix credo --strict` and `mix dialyzer` (after `mix deps.get && mix compile`).
8+
9+
## Coding Style & Naming Conventions
10+
Stick to idiomatic Elixir: two-space indentation, pipeline-friendly code, and one module per file (e.g. `lib/pipelines_api/web/router.ex` for `PipelinesAPI.Web.Router`). Run `mix format` before opening a PR; `.formatter.exs` defines the inputs. Let Credo guide readability—fix warnings rather than ignoring them. Prefer descriptive atoms, snake_case for functions, PascalCase for modules, and add concise `@moduledoc` notes to complex modules.
11+
12+
## Testing Guidelines
13+
The project uses ExUnit with helpers in `test/support`. Place new suites under `test/`, mirroring the source path, and name files `_test.exs`. Run `mix test --cover` when touching request or authorization logic to watch coverage. For gRPC integrations, lean on `GrpcMock` rather than real services. Keep tests deterministic by stubbing network calls and timestamps with `Faker` or recorded fixtures.
14+
15+
## Commit & Pull Request Guidelines
16+
Follow the Conventional Commits pattern seen in history (`type(scope): summary`), using scopes such as `secrethub` or `docs` to clarify impact. Keep commits focused and include test or lint updates when relevant. Pull requests must describe the scenario, list impacted gRPC endpoints, and note any Helm or config follow-up. Link tracking issues, call out breaking changes, and include the latest `mix test` or `mix credo` output before requesting review.
17+
18+
## Security & Configuration Tips
19+
Environment defaults live in the Makefile and `docker-compose.yml`; override gRPC endpoints via variables like `LOGHUB_API_URL` or `API_VERSION`. Never hardcode secrets—use the provided env vars or the helpers in `lib/pipelines_api/secrets`. When updating protobuf definitions, ensure regenerated clients do not expose new fields without matching authorization checks in the HTTP layer.
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# Repository Architecture Notes
2+
3+
## Service Purpose & Entry Points
4+
Pipelines API is the public HTTP façade for Semaphore’s Pipelines domain. It receives REST requests, validates them, and forwards calls to internal gRPC services such as Pipelines, Workflows, Deployments, RBAC, and SecretHub. The OTP entry point is `lib/pipelines_api/application.ex`, which starts a `Plug.Cowboy` endpoint on port 4004, initialises feature providers, and warms two `Cachex` caches (`:feature_provider_cache`, `:project_api_cache`).
5+
6+
## Request Flow & Runtime Stack
7+
- HTTP traffic lands in `lib/pipelines_api/router.ex`, a `Plug.Router` that wires every route to a domain-specific module (e.g., `PipelinesAPI.Pipelines.Describe`).
8+
- Domain modules follow a consistent pattern: parse params, call into a `*_client` module, and reply via helpers in `PipelinesAPI.Pipelines.Common` or a domain-specific responder. Success and error tuples map to HTTP status automatically.
9+
- Authentication/authorization gates live in the RBAC and Feature clients; there are no custom Plug modules today (`lib/pipelines_api/plugs/` is intentionally empty).
10+
- Long-running gRPC calls run inside `Wormhole.capture` to enforce timeouts defined in `config/config.exs`. Metrics flow through `PipelinesAPI.Util.Metrics` into Watchman/StatsD (`watchman` config block).
11+
12+
## Directory Map
13+
- `lib/pipelines_api/pipelines`, `workflows`, `deployments`, `schedules`, `self_hosted_agent_types`, etc.: HTTP handlers grouped by resource. Files mirror verbs (`list.ex`, `describe.ex`, `terminate.ex`).
14+
- `lib/pipelines_api/*_client/`: Each client encapsulates gRPC glue with submodules for request formatting, the actual gRPC stub wrapper, and response shaping. Single-file clients (e.g., `jobs_client.ex`) follow the same tuple contract.
15+
- `lib/internal_api/`: Generated gRPC stubs. Regenerate via `make pb.gen`, which clones `renderedtext/internal_api` and runs `scripts/internal_protos.sh`.
16+
- `config/`: Compile-time (`config.exs`) and runtime (`runtime.exs`) settings. `config/test.exs` shortens gRPC and Wormhole timeouts; `config/dev.exs` points metrics to the local StatsD agent.
17+
- `test/support/`: Shared stubs and fake services. `Support.FakeServices` boots `GrpcMock` servers on port 50052 so unit tests never hit production systems.
18+
- `scripts/`: `internal_protos.sh` for protobuf regeneration and `vagrant_sudo` helper for privileged Docker commands.
19+
20+
## External Integrations
21+
- Environment variables in `Makefile` and `docker-compose.yml` provide endpoints (`PPL_GRPC_URL`, `LOGHUB_API_URL`, `FEATURE_GRPC_URL`, etc.). Override them when connecting to non-default clusters.
22+
- Feature flags are served by a remote Feature Hub unless `ON_PREM=true`, in which case a YAML provider is loaded with `FEATURE_YAML_PATH`.
23+
- Response pagination goes through `Scrivener.Headers`, which rewrites paths to `/api/<API_VERSION>/…` before responses leave the service.
24+
25+
## Development & Diagnostics Workflow
26+
- `make console` launches the Docker dev container with dependencies and shares `_build`/`deps` for fast recompiles.
27+
- From inside the container: start the API via `iex -S mix`. Health probes hit `/` or `/health_check/ping`.
28+
- Format and lint with `mix format` and `mix credo --strict`. Optional type checks come from `mix dialyzer` once PLTs are cached.
29+
- Run suites with `make unit-test` or `mix test --cover`. The custom ExUnit formatter writes JUnit XML reports under `./out/test-reports.xml`.
30+
- To inspect gRPC traffic locally, tail logs produced by `PipelinesAPI.Util.Log` or enable DEBUG by exporting `LOG_LEVEL=debug` before boot.
31+
32+
## Testing Infrastructure
33+
- Tests rely heavily on support factories (`test/support/stubs.ex`, `test/support/factories.ex`) to fabricate workflows, pipelines, and users.
34+
- `GrpcMock` doubles are registered for every dependency (SecretHub, Gofer, Pipeline, ProjectHub, etc.) in `Support.FakeServices.init/0`.
35+
- When adding new gRPC calls, extend the relevant mock to cover the new RPC and update helper stubs so fixtures stay meaningful.
36+
37+
## Common Change Playbooks
38+
1. **Add or adjust an endpoint**: Update `router.ex`, create/modify the domain module under `lib/pipelines_api/<resource>/`, and ensure responses return `{status, payload}` tuples through the common helper.
39+
2. **Add a gRPC call**: Touch the appropriate `*_client` directory—update the RequestFormatter, extend the `GrpcClient` wrapper, and cover ResponseFormatter cases. Regenerate protobuf stubs if the contract changed.
40+
3. **Introduce feature-flagged behaviour**: Depend on the provider returned from `Application.get_env(:pipelines_api, :feature_provider)`, and store expensive lookups in Cachex to match existing patterns.
41+
4. **Regenerate protobufs**: Run `make pb.gen`, commit resulting changes under `lib/internal_api`, and verify no manual edits are lost.
42+
5. **Triage production incidents**: Use `/logs/:job_id` for streaming logs, `/troubleshoot/*` endpoints for aggregated context, inspect Watchman metrics for latency spikes, and confirm caches or feature toggles aren’t stale.
43+
44+
Keep this document nearby when picking up new tasks—most flows follow the patterns above, so identifying the right directory or client is usually the quickest path to a fix.

public-api/v1alpha/Dockerfile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ ENV MIX_ENV=$BUILD_ENV
1212

1313
RUN echo "Build for $MIX_ENV environment started"
1414

15-
RUN apk update && apk add --no-cache build-base git python3 curl openssh
15+
RUN apk update && apk add --no-cache build-base git python3 curl openssh bash
1616

1717
RUN mkdir -p ~/.ssh
1818
RUN touch ~/.ssh/known_hosts

public-api/v1alpha/docker-compose.yml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,4 +45,6 @@ services:
4545
ON_PREM: ${ON_PREM:-false}
4646

4747
volumes:
48-
- .:/app
48+
- .:/app
49+
- /app/_build
50+
- /app/deps

public-api/v1alpha/lib/pipelines_api/gofer_client/request_formatter.ex

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,16 @@ defmodule PipelinesAPI.GoferClient.RequestFormatter do
3131

3232
defp to_int(val, _field) when is_integer(val), do: val
3333

34-
defp to_int(val, field) do
34+
defp to_int(val, field) when is_binary(val) do
35+
case Integer.parse(val) do
36+
{int, ""} -> int
37+
_ -> invalid_integer(field, val)
38+
end
39+
end
40+
41+
defp to_int(val, field), do: invalid_integer(field, val)
42+
43+
defp invalid_integer(field, val) do
3544
"Invalid value of '#{field}' param: #{inspect(val)} - needs to be integer."
3645
|> ToTuple.user_error()
3746
|> throw()

public-api/v1alpha/lib/pipelines_api/periodic_scheduler_client/request_fromatter.ex

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,16 @@ defmodule PipelinesAPI.PeriodicSchedulerClient.RequestFormatter do
118118

119119
defp to_int(val, _field) when is_integer(val), do: val
120120

121-
defp to_int(val, field) do
121+
defp to_int(val, field) when is_binary(val) do
122+
case Integer.parse(val) do
123+
{int, ""} -> int
124+
_ -> invalid_integer(field, val)
125+
end
126+
end
127+
128+
defp to_int(val, field), do: invalid_integer(field, val)
129+
130+
defp invalid_integer(field, val) do
122131
"Invalid value of '#{field}' param: #{inspect(val)} - needs to be integer."
123132
|> ToTuple.user_error()
124133
|> throw()

public-api/v1alpha/test/gofer_client/request_formatter_test.exs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ defmodule PipelinesAPI.GoferClient.RequestFormatter.Test do
55

66
alias InternalApi.Gofer.{
77
TriggerRequest,
8-
EnvVariable
8+
EnvVariable,
9+
ListTriggerEventsRequest
910
}
1011

1112
test "form_trigger_request() returns internal error when it is not called with map as a param" do
@@ -52,4 +53,27 @@ defmodule PipelinesAPI.GoferClient.RequestFormatter.Test do
5253
assert {:error, {:user, msg}} = RequestFormatter.form_trigger_request(params)
5354
assert msg == "Invalid value of 'override' param: \"not-a-bool-val\" - needs to be boolean."
5455
end
56+
57+
test "form_list_request() converts numeric params received as strings" do
58+
params = %{
59+
"switch_id" => "sw1",
60+
"name" => "tg1",
61+
"page" => "2",
62+
"page_size" => "15"
63+
}
64+
65+
assert {:ok,
66+
%ListTriggerEventsRequest{
67+
switch_id: "sw1",
68+
target_name: "tg1",
69+
page: 2,
70+
page_size: 15
71+
}} = RequestFormatter.form_list_request(params)
72+
end
73+
74+
test "form_list_request() returns user error when numeric params are invalid" do
75+
assert {:error, {:user, msg}} = RequestFormatter.form_list_request(%{"page_size" => "ten"})
76+
77+
assert msg == "Invalid value of 'page_size' param: \"ten\" - needs to be integer."
78+
end
5579
end

public-api/v1alpha/test/periodic_scheduler_client/request_formatter_test.exs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,19 @@ defmodule PipelinesAPI.PeriodicSchedulerClient.RequestFormatter.Test do
119119
assert request.requester_id == ""
120120
end
121121

122+
test "form_list_request() converts pagination params provided as strings" do
123+
conn = create_conn(:describe)
124+
125+
params = %{
126+
"project_id" => UUID.uuid4(),
127+
"page" => "3",
128+
"page_size" => "25"
129+
}
130+
131+
assert {:ok, %ListRequest{page: 3, page_size: 25}} =
132+
RequestFormatter.form_list_request(params, conn)
133+
end
134+
122135
# Run Now
123136

124137
test "form_run_now_request() returns internal error when it is not called with map as a param" do

0 commit comments

Comments
 (0)