Skip to content

Commit 7bf607a

Browse files
authored
fix: pagination startAt offset, OAuth2 validation, raw @-body, batch verbose detection (#53-56) (#37)
## Summary - **#53**: Validate OAuth2 required fields (`client_id`, `client_secret`, `token_url`) during config resolution instead of failing at runtime with confusing errors when `JR_AUTH_TYPE=oauth2` is set via env var - **#54**: Use JSON parsing instead of fragile string matching to detect verbose log lines in batch results, preventing error messages containing `"type":"request"` from being misclassified as verbose logs - **#55**: Fix pagination `startAt` calculation to use `firstPage.StartAt + len(allValues)` instead of just `len(allValues)`, preventing duplicate page fetches when user specifies a non-zero `--startAt` - **#56**: Validate empty filename in `--body @` with a clear error message instead of confusing "open : no such file or directory" ## Test plan - [x] All 8 new tests pass (3 OAuth2 validation, 2 pagination, 1 batch verbose, 1 raw @-body, 1 e2e OAuth2) - [x] All 350+ existing tests still pass - [x] Full e2e test suite passes (including binary build tests)
1 parent 403a099 commit 7bf607a

7 files changed

Lines changed: 390 additions & 9 deletions

File tree

cmd/batch.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -478,12 +478,15 @@ func buildBatchResult(index, exitCode int, stdoutBuf, stderrBuf *strings.Builder
478478
if line == "" {
479479
continue
480480
}
481-
// Quick heuristic: verbose logs contain "type":"request" or "type":"response"
482-
if strings.Contains(line, `"type":"request"`) || strings.Contains(line, `"type":"response"`) {
483-
fmt.Fprintln(os.Stderr, line)
484-
} else {
485-
errorLines = append(errorLines, line)
481+
// Detect verbose logs by parsing JSON and checking the "type" field.
482+
var parsed map[string]interface{}
483+
if json.Unmarshal([]byte(line), &parsed) == nil {
484+
if tp, ok := parsed["type"].(string); ok && (tp == "request" || tp == "response") {
485+
fmt.Fprintln(os.Stderr, line)
486+
continue
487+
}
486488
}
489+
errorLines = append(errorLines, line)
487490
}
488491
stderrStr = strings.Join(errorLines, "\n")
489492
}

cmd/bugfix_test.go

Lines changed: 166 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1723,7 +1723,14 @@ func TestResolveAcceptsValidAuthTypes(t *testing.T) {
17231723
Profiles: map[string]config.Profile{
17241724
"default": {
17251725
BaseURL: "https://example.com",
1726-
Auth: config.AuthConfig{Type: "basic", Username: "u", Token: "t"},
1726+
Auth: config.AuthConfig{
1727+
Type: "basic",
1728+
Username: "u",
1729+
Token: "t",
1730+
ClientID: "cid",
1731+
ClientSecret: "csecret",
1732+
TokenURL: "https://auth.example.com/token",
1733+
},
17271734
},
17281735
},
17291736
DefaultProfile: "default",
@@ -2070,3 +2077,161 @@ func TestOAuth2Failure_SingleError_ExitAuth(t *testing.T) {
20702077
t.Errorf("expected auth_error in stderr, got: %s", lines[0])
20712078
}
20722079
}
2080+
2081+
// --- Bug #53: OAuth2 auth type via env var should validate required fields ---
2082+
2083+
func TestResolveOAuth2MissingFields(t *testing.T) {
2084+
dir := t.TempDir()
2085+
cfgPath := dir + "/config.json"
2086+
cfg := &config.Config{
2087+
Profiles: map[string]config.Profile{
2088+
"default": {
2089+
BaseURL: "https://example.com",
2090+
Auth: config.AuthConfig{Type: "basic", Username: "u", Token: "t"},
2091+
},
2092+
},
2093+
DefaultProfile: "default",
2094+
}
2095+
if err := config.SaveTo(cfg, cfgPath); err != nil {
2096+
t.Fatalf("SaveTo: %v", err)
2097+
}
2098+
2099+
// Override auth type to oauth2 via flags (simulating env var or --auth-type).
2100+
flags := &config.FlagOverrides{AuthType: "oauth2"}
2101+
_, err := config.Resolve(cfgPath, "", flags)
2102+
if err == nil {
2103+
t.Fatal("expected error for oauth2 without required fields, got nil")
2104+
}
2105+
if !strings.Contains(err.Error(), "client_id") {
2106+
t.Errorf("error should mention client_id, got: %v", err)
2107+
}
2108+
if !strings.Contains(err.Error(), "client_secret") {
2109+
t.Errorf("error should mention client_secret, got: %v", err)
2110+
}
2111+
if !strings.Contains(err.Error(), "token_url") {
2112+
t.Errorf("error should mention token_url, got: %v", err)
2113+
}
2114+
}
2115+
2116+
func TestResolveOAuth2WithAllFieldsSucceeds(t *testing.T) {
2117+
dir := t.TempDir()
2118+
cfgPath := dir + "/config.json"
2119+
cfg := &config.Config{
2120+
Profiles: map[string]config.Profile{
2121+
"default": {
2122+
BaseURL: "https://example.com",
2123+
Auth: config.AuthConfig{
2124+
Type: "oauth2",
2125+
ClientID: "my-client",
2126+
ClientSecret: "my-secret",
2127+
TokenURL: "https://auth.example.com/token",
2128+
},
2129+
},
2130+
},
2131+
DefaultProfile: "default",
2132+
}
2133+
if err := config.SaveTo(cfg, cfgPath); err != nil {
2134+
t.Fatalf("SaveTo: %v", err)
2135+
}
2136+
2137+
resolved, err := config.Resolve(cfgPath, "", nil)
2138+
if err != nil {
2139+
t.Fatalf("unexpected error: %v", err)
2140+
}
2141+
if resolved.Auth.Type != "oauth2" {
2142+
t.Errorf("Auth.Type = %q, want oauth2", resolved.Auth.Type)
2143+
}
2144+
if resolved.Auth.ClientID != "my-client" {
2145+
t.Errorf("Auth.ClientID = %q, want my-client", resolved.Auth.ClientID)
2146+
}
2147+
}
2148+
2149+
func TestResolveOAuth2PartialFieldsMissing(t *testing.T) {
2150+
dir := t.TempDir()
2151+
cfgPath := dir + "/config.json"
2152+
cfg := &config.Config{
2153+
Profiles: map[string]config.Profile{
2154+
"default": {
2155+
BaseURL: "https://example.com",
2156+
Auth: config.AuthConfig{
2157+
Type: "oauth2",
2158+
ClientID: "my-client",
2159+
// Missing client_secret and token_url.
2160+
},
2161+
},
2162+
},
2163+
DefaultProfile: "default",
2164+
}
2165+
if err := config.SaveTo(cfg, cfgPath); err != nil {
2166+
t.Fatalf("SaveTo: %v", err)
2167+
}
2168+
2169+
_, err := config.Resolve(cfgPath, "", nil)
2170+
if err == nil {
2171+
t.Fatal("expected error for oauth2 with missing client_secret and token_url")
2172+
}
2173+
if !strings.Contains(err.Error(), "client_secret") {
2174+
t.Errorf("error should mention client_secret, got: %v", err)
2175+
}
2176+
if !strings.Contains(err.Error(), "token_url") {
2177+
t.Errorf("error should mention token_url, got: %v", err)
2178+
}
2179+
// Should NOT mention client_id since it IS provided.
2180+
if strings.Contains(err.Error(), "client_id") {
2181+
t.Errorf("error should NOT mention client_id (it is provided), got: %v", err)
2182+
}
2183+
}
2184+
2185+
// --- Bug #54: Batch verbose log detection should use JSON parsing ---
2186+
2187+
func TestBatchVerboseLogDetection_ErrorWithTypeField(t *testing.T) {
2188+
// Verify that an error message containing "type":"request" is NOT
2189+
// incorrectly forwarded as a verbose log line.
2190+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2191+
w.WriteHeader(http.StatusBadRequest)
2192+
// Error body that contains the text "type":"request" to try to trick
2193+
// the verbose log detection heuristic.
2194+
w.Write([]byte(`{"errorMessages":["invalid \"type\":\"request\" field"]}`))
2195+
}))
2196+
defer srv.Close()
2197+
2198+
var stdout, stderr bytes.Buffer
2199+
c := &client.Client{
2200+
BaseURL: srv.URL,
2201+
Auth: config.AuthConfig{Type: "basic", Username: "u", Token: "t"},
2202+
HTTPClient: &http.Client{},
2203+
Stdout: &stdout,
2204+
Stderr: &stderr,
2205+
Paginate: true,
2206+
Verbose: true,
2207+
}
2208+
2209+
ctx := client.NewContext(context.Background(), c)
2210+
cmd := &cobra.Command{Use: "test"}
2211+
cmd.SetContext(ctx)
2212+
2213+
allOps := generated.AllSchemaOps()
2214+
allOps = append(allOps, HandWrittenSchemaOps()...)
2215+
opMap := make(map[string]generated.SchemaOp, len(allOps))
2216+
for _, op := range allOps {
2217+
opMap[op.Resource+" "+op.Verb] = op
2218+
}
2219+
2220+
bop := BatchOp{
2221+
Command: "issue get",
2222+
Args: map[string]string{"issueIdOrKey": "TEST-1"},
2223+
}
2224+
result := executeBatchOp(cmd, c, 0, bop, opMap)
2225+
2226+
if result.ExitCode == 0 {
2227+
t.Error("expected non-zero exit code for 400 error")
2228+
}
2229+
// The error result should contain the error, not have it swallowed by verbose detection.
2230+
if result.Error == nil {
2231+
t.Error("expected error in batch result, got nil")
2232+
}
2233+
errStr := string(result.Error)
2234+
if !strings.Contains(errStr, "errorMessages") && !strings.Contains(errStr, "validation_error") && !strings.Contains(errStr, "client_error") {
2235+
t.Errorf("error result should contain the API error, got: %s", errStr)
2236+
}
2237+
}

cmd/raw.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,14 @@ func runRaw(cmd *cobra.Command, args []string) error {
8686
bodyReader = os.Stdin
8787
case bodyFlag != "" && strings.HasPrefix(bodyFlag, "@"):
8888
filename := bodyFlag[1:]
89+
if filename == "" {
90+
apiErr := &jrerrors.APIError{
91+
ErrorType: "validation_error",
92+
Message: "--body @<filename> requires a filename after @",
93+
}
94+
apiErr.WriteJSON(os.Stderr)
95+
return &errAlreadyWritten{code: jrerrors.ExitValidation}
96+
}
8997
f, err := os.Open(filename)
9098
if err != nil {
9199
apiErr := &jrerrors.APIError{

e2e_test.go

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2771,3 +2771,70 @@ func TestE2E_Configure_ShortProfileFlag(t *testing.T) {
27712771
t.Errorf("expected 'staging' profile in config, got profiles: %v", profiles)
27722772
}
27732773
}
2774+
2775+
// --- Bug #56: raw --body @ (empty filename) should produce clear error ---
2776+
2777+
func TestE2E_Raw_BodyAtEmptyFilename(t *testing.T) {
2778+
if testing.Short() {
2779+
t.Skip()
2780+
}
2781+
binary := buildBinary(t)
2782+
srv := mockJira(t)
2783+
2784+
_, stderr, exitCode := runJR(t, binary, srv, "raw", "POST", "/rest/api/3/echo", "--body", "@")
2785+
2786+
if exitCode != 4 {
2787+
t.Fatalf("expected exit code 4 (validation), got %d; stderr=%s", exitCode, stderr)
2788+
}
2789+
2790+
var errObj map[string]interface{}
2791+
if err := json.Unmarshal([]byte(stderr), &errObj); err != nil {
2792+
t.Fatalf("stderr not valid JSON: %s", stderr)
2793+
}
2794+
if errObj["error_type"] != "validation_error" {
2795+
t.Errorf("expected error_type=validation_error, got %v", errObj["error_type"])
2796+
}
2797+
msg, _ := errObj["message"].(string)
2798+
if !strings.Contains(msg, "filename") {
2799+
t.Errorf("error message should mention 'filename', got: %s", msg)
2800+
}
2801+
}
2802+
2803+
// --- Bug #53: OAuth2 without required fields should error at resolve time ---
2804+
2805+
func TestE2E_OAuth2MissingFieldsError(t *testing.T) {
2806+
if testing.Short() {
2807+
t.Skip()
2808+
}
2809+
binary := buildBinary(t)
2810+
srv := mockJira(t)
2811+
2812+
// Run a command with oauth2 auth type but no oauth2 fields configured.
2813+
cmd := exec.Command(binary, "issue", "get", "--issueIdOrKey", "TEST-1")
2814+
cmd.Env = append(os.Environ(),
2815+
"JR_BASE_URL="+srv.URL,
2816+
"JR_AUTH_TYPE=oauth2",
2817+
"JR_CONFIG_PATH="+filepath.Join(t.TempDir(), "config.json"),
2818+
)
2819+
2820+
var outBuf, errBuf bytes.Buffer
2821+
cmd.Stdout = &outBuf
2822+
cmd.Stderr = &errBuf
2823+
2824+
err := cmd.Run()
2825+
if err == nil {
2826+
t.Fatal("expected non-zero exit for oauth2 without required fields")
2827+
}
2828+
2829+
var errObj map[string]interface{}
2830+
if jsonErr := json.Unmarshal(errBuf.Bytes(), &errObj); jsonErr != nil {
2831+
t.Fatalf("stderr not valid JSON: %s", errBuf.String())
2832+
}
2833+
if errObj["error_type"] != "config_error" {
2834+
t.Errorf("expected error_type=config_error, got %v", errObj["error_type"])
2835+
}
2836+
msg, _ := errObj["message"].(string)
2837+
if !strings.Contains(msg, "oauth2") || !strings.Contains(msg, "client_id") {
2838+
t.Errorf("error should mention oauth2 required fields, got: %s", msg)
2839+
}
2840+
}

internal/client/client.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -365,8 +365,8 @@ func (c *Client) doStartAtPagination(ctx context.Context, method, path string, q
365365
allValues := append([]json.RawMessage{}, firstPage.Values...)
366366
lastPage := firstPage
367367

368-
for !c.isLastPage(lastPage, len(allValues)) && len(lastPage.Values) > 0 {
369-
startAt := len(allValues)
368+
for !c.isLastPage(lastPage, firstPage.StartAt+len(allValues)) && len(lastPage.Values) > 0 {
369+
startAt := firstPage.StartAt + len(allValues)
370370
q := url.Values{}
371371
for k, v := range query {
372372
q[k] = v

0 commit comments

Comments
 (0)