From e2d481e7b8ea0d950d1f71f216207ad00a75c6af Mon Sep 17 00:00:00 2001 From: Andriy Oblivantsev Date: Fri, 24 Apr 2026 12:36:40 +0100 Subject: [PATCH] refactor: DRY auth path, fix Sprintf/RE2 bugs; real integration tests only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why --- - Sprintf(*p.Title) fed the title as a format string — % in titles broke the String() method; also panicked on nil Title. - internal/applications.buildSummary used RE2-unsupported `(?= ...)` lookahead inside regexp.MustCompile, panicking the first time the applications-sync path was exercised on Go 1.23+. - Query() duplicated the auth-expiry check inline while ensureToken() already handled it — two code paths drifted. - Request.Debug split Query() into two branches that both unmarshalled into the same target value. Dead code. - httptest fixtures that emulated OnlyOffice endpoints were lying to us: they passed locally yet never caught a single real protocol regression. What ---- - Project.String(): nil-safe, no Sprintf format-string interpretation. - buildSummary regex: RE2-safe non-capturing trailing delimiter `(?:\n## |$)` replaces the lookahead. - Query() routes through ensureToken(); body marshalling factored into an unexported requestBodyReader(). Debug flag retained for backwards compatibility, documented as a no-op, to be removed at next major. - Dropped Debug: true stray flags in GetTasks/UpdateProjectTask. - Deleted httptest-based OnlyOffice mocks. unit_test.go is now pure Go (parsers, helpers, env aliases, ctx cancellation against an unroutable address). client_test.go is `//go:build integration` and runs against a real OnlyOffice, skipping cleanly without ONLYOFFICE_URL/USER/PASS. - AGENTS.md + .cursor/rules/no-synthetic-mocks.mdc document the new testing policy. Verified -------- - `go test ./...` green (15 unit tests across package + internal). - `go test -tags=integration ./...` green against live office.produktor.io (5 integration tests: auth, projects, lifecycle, calendar+CRM read, task list). - inventar-sync smoke dry-run against live OO project 33 + Gitea found 30 tasks, 0 mutations. Made-with: Cursor --- .cursor/rules/no-synthetic-mocks.mdc | 38 ++++ AGENTS.md | 20 +- CHANGELOG.md | 32 +++ client_test.go | 292 +++++++++++++++++--------- httpx_test.go | 270 ------------------------ internal/applications/applications.go | 30 +-- internal/applications/parse_test.go | 31 +++ onlyoffice.go | 109 +++++----- unit_test.go | 133 ++++++++++++ 9 files changed, 508 insertions(+), 447 deletions(-) create mode 100644 .cursor/rules/no-synthetic-mocks.mdc delete mode 100644 httpx_test.go create mode 100644 unit_test.go diff --git a/.cursor/rules/no-synthetic-mocks.mdc b/.cursor/rules/no-synthetic-mocks.mdc new file mode 100644 index 0000000..a334fbd --- /dev/null +++ b/.cursor/rules/no-synthetic-mocks.mdc @@ -0,0 +1,38 @@ +--- +description: No synthetic OnlyOffice/Gitea mocks; prefer real integration tests +globs: + - "**/*_test.go" +alwaysApply: false +--- +# Testing policy — no synthetic vendor mockups + +When authoring tests under `github.com/eslider/go-onlyoffice`, do **not** +build `httptest.NewServer` fixtures that emulate OnlyOffice, Gitea, or any +other third-party API. Simulated vendor responses drift from reality, give +false green signals, and hide protocol changes. + +## What to do instead + +1. **Unit tests** — pure Go, no network. Use them for parsers, encoders, + struct conversions, pure helpers. No `httptest` that fakes the vendor. +2. **Integration tests** — `//go:build integration` tag in a `*_integration_test.go` + file. Read credentials from env: + - `ONLYOFFICE_URL` / `ONLYOFFICE_HOST` + - `ONLYOFFICE_USER` / `ONLYOFFICE_NAME` + - `ONLYOFFICE_PASS` / `ONLYOFFICE_PASSWORD` + Call `t.Skip("ONLYOFFICE_URL not set")` when credentials are absent so the + regular `go test ./...` stays green in CI. +3. **Run integration**: `go test -tags=integration ./...`. +4. **Every new endpoint** ships with an integration test in the same PR. + +## Narrow exception + +`httptest.NewServer` is OK when verifying the **caller's own** HTTP +behaviour (e.g. a user's handler or middleware we are wrapping). It is **not** +OK when the test server is pretending to be OnlyOffice or Gitea. + +## Migrating existing tests + +If you find a test that handles routes like `/api/2.0/...` and returns canned +JSON, convert it to an integration test (or delete it if the behaviour is +already covered by integration). diff --git a/AGENTS.md b/AGENTS.md index b3f7629..4144307 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,9 +13,27 @@ Canonical Go client for OnlyOffice Workspace (Projects + Calendar + CRM) and the - Library must never call `godotenv.Load()` — the CLI does that. - New endpoints go into the library first; CLI commands are thin wrappers. - No secrets in the repo; use `.env` (gitignored). Commit `.env.example` only. -- Prefer httptest-based unit tests (see `httpx_test.go`); integration tests (`client_test.go`) auto-skip without `ONLYOFFICE_URL`. - Follow SemVer on tags; this repo is tagged at GitHub under `git@github.com:eSlider/go-onlyoffice.git`. +### Testing policy (2026-04-24) + +**No synthetic OnlyOffice mockups.** Protocol-level behaviour must be +verified against a real OnlyOffice instance. `httptest.NewServer` is only +acceptable for testing the *caller's* logic that the library can't reach +(for example, the user's own HTTP handler). Anywhere we would otherwise +write `mux.HandleFunc("/api/2.0/...")` to emulate OnlyOffice, we write an +**integration test** instead. + +- Unit tests (`*_test.go`, no build tag) — pure Go: parsers, encoders, + struct conversions. No network. No fake servers that emulate the vendor. +- Integration tests (`//go:build integration` tag in `*_integration_test.go`) + — hit a live OnlyOffice instance. Credentials come from `ONLYOFFICE_URL`, + `ONLYOFFICE_USER`, `ONLYOFFICE_PASS` (aliases `_HOST`/`_NAME`/`_PASSWORD` + also accepted). Tests **skip** cleanly when credentials are missing so + `go test ./...` remains green in CI. +- Run integration with: `go test -tags=integration ./...`. +- New endpoints **must** ship with an integration test before merge. + ## Related - [`eSlider/inventar`](https://git.produktor.io/eSlider/inventar) — ASR/ADR (see ASR-0008 Go library module conventions). diff --git a/CHANGELOG.md b/CHANGELOG.md index 00295e7..f51fa63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,38 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +## [0.3.2] - 2026-04-24 + +### Fixed + +- `Project.String()` no longer interprets the title as a format string + (`fmt.Sprintf(*p.Title)`) and is now nil-safe on a zero-value `Project`. +- `internal/applications.buildSummary` no longer panics at regex compile time + on Go 1.23+ — the previous `(?= ...)` lookahead is replaced with an RE2-safe + non-capturing trailing delimiter. + +### Changed + +- `Client.Query` now routes token acquisition through the shared + `ensureToken` path instead of duplicating the auth-expiry check inline. +- Request body marshalling is consolidated into an unexported + `requestBodyReader` helper (DRY; no change to the public surface). +- The `Request.Debug` field is preserved for backwards compatibility but no + longer changes behaviour — both branches used to unmarshal into the same + target value. We'll remove the field in a future major release. + +### Tests + +- Deleted `httptest.NewServer` fixtures that emulated OnlyOffice protocol + endpoints. Replaced them with: + - pure-Go unit tests in `unit_test.go` (no network); + - real integration tests in `client_test.go` guarded by + `//go:build integration`. Run with + `go test -tags=integration ./...`. Tests skip cleanly when + `ONLYOFFICE_URL/USER/PASS` (or aliases) are absent. +- New policy documented in `AGENTS.md` and + `.cursor/rules/no-synthetic-mocks.mdc`. + ## [0.3.1] - 2026-04-24 ### Added diff --git a/client_test.go b/client_test.go index 68c58ab..d799527 100644 --- a/client_test.go +++ b/client_test.go @@ -1,128 +1,220 @@ +//go:build integration + package onlyoffice +// Integration tests — hit a live OnlyOffice Workspace instance. Credentials +// come from ONLYOFFICE_URL / ONLYOFFICE_USER / ONLYOFFICE_PASS (aliases +// _HOST / _NAME / _PASSWORD also accepted). Without credentials every test +// here skips. +// +// Run with: +// +// go test -tags=integration ./... +// +// These tests are destructive on the target instance. They create projects +// with titles prefixed "go-onlyoffice-test-" and clean up afterwards. Do not +// run against an instance you don't own. + import ( - "os" + "context" + "strconv" "strings" "testing" "time" - - "github.com/joho/godotenv" ) -// Load environment variables -func init() { - // Load from .env file in the project root - _ = godotenv.Load(".env") -} - -func skipWithoutCredentials(t *testing.T) { +func skipWithoutCredentials(t *testing.T) Credentials { t.Helper() - if os.Getenv("ONLYOFFICE_URL") == "" { - t.Skip("ONLYOFFICE_URL is not set, skipping integration test") + c := GetEnvironmentCredentials() + if c.Url == "" || c.User == "" || c.Password == "" { + t.Skip("ONLYOFFICE_URL/USER/PASS not set — skipping integration test") } + return c } -func TestNewClient(t *testing.T) { - skipWithoutCredentials(t) +func liveClient(t *testing.T) *Client { + t.Helper() + c := NewClient(skipWithoutCredentials(t)) + c.SetDefaults(GetEnvironmentDefaults()) + return c +} - // Create a new OnlyOffice client - credentials := GetEnvironmentCredentials() - client := NewClient(credentials) - var token *Token - var err error - token, err = client.Auth(&credentials) +const testProjectPrefix = "go-onlyoffice-test-" +func cleanupTestProjects(t *testing.T, c *Client) { + t.Helper() + projects, err := c.GetProjects() if err != nil { - t.Errorf("Failed to get token: %v", err) + t.Logf("cleanup: GetProjects: %v", err) + return } - if token == nil { - t.Error("Value is empty") - } - - // Clean up: remove all projects start with "Test project" - projects, err := client.GetProjects() - if err != nil { - t.Errorf("Failed to get projects: %v", err) - } - - for _, project := range projects { - if strings.HasPrefix(*project.Title, "Test project") { - prjStatus, err := client.DeleteProject(*project.ID) - if err != nil { - t.Errorf("Failed to delete project: %v", err) - } - if *prjStatus.ID != *project.ID { - t.Errorf("Project ID is not equal: %v != %v", prjStatus.ID, project.ID) + for _, p := range projects { + if p.Title != nil && strings.HasPrefix(*p.Title, testProjectPrefix) { + if _, err := c.DeleteProject(*p.ID); err != nil { + t.Logf("cleanup: DeleteProject %d: %v", *p.ID, err) } } } +} - // Test create project - project, err := client.CreateProject(NewProjectRequest{ - Title: "Test project", - Description: "Test project description", - }) - - if err != nil { - t.Errorf("Failed to create project: %v", err) +func TestIntegrationAuthenticateContext(t *testing.T) { + c := liveClient(t) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := c.AuthenticateContext(ctx); err != nil { + t.Fatalf("AuthenticateContext: %v", err) } - - // Update project - project, err = client.UpdateProject(ProjectUpdateRequest{ - ID: *project.ID, - Title: "Test project updated", - ResponsibleID: *project.Responsible.ID, - }) - - var task *Task - // Test create project task - task, err = client.CreateProjectTask(NewProjectTaskRequest{ - ProjectId: *project.ID, - Title: "Test task", - Description: "Test task description", - Notify: true, - MilestoneId: 0, - Priority: 0, - // Deadline +2 days - StartDate: Time(time.Now().AddDate(0, 0, -2)), - Deadline: Time(time.Now().AddDate(0, 0, 2)), - }) - - // Update project task - startDate := Time(time.Now().AddDate(0, 0, -14)) - deadline := Time(time.Now().AddDate(0, 0, 3)) - task, err = client.UpdateProjectTask(ProjectTaskUpdateRequest{ - ID: *task.ID, - Title: "Test task updated", - Description: "Test task description updated", - StartDate: &startDate, - Deadline: &deadline, - }) - - if err != nil { - t.Errorf("Failed to create task: %v", err) + if c.token == nil || c.token.Value == "" { + t.Fatal("token not cached after AuthenticateContext") } - - if task == nil { - t.Error("Value is empty") + // Second call must hit the cache. + cachedValue := c.token.Value + if err := c.AuthenticateContext(ctx); err != nil { + t.Fatalf("AuthenticateContext (cached): %v", err) } - - // Delete project - prj, err := client.DeleteProject(*project.ID) - if err != nil { - t.Errorf("Failed to delete project: %v", err) + if c.token.Value != cachedValue { + t.Fatal("cached token was replaced unexpectedly") } - - if *prj.ID != *project.ID { - t.Errorf("Project ID is not equal: %v != %v", prj.ID, project.ID) + // Invalidate forces re-auth. + c.InvalidateToken() + if c.token != nil { + t.Fatal("InvalidateToken did not clear the cache") } - - // Test list projects - projects, err = client.GetProjects() - if err != nil { - t.Errorf("Failed to get projects: %v", err) + if err := c.AuthenticateContext(ctx); err != nil { + t.Fatalf("AuthenticateContext (after invalidate): %v", err) } - if len(projects) == 0 { - t.Error("Value is empty") + if c.token == nil { + t.Fatal("post-invalidate auth left token nil") + } +} + +func TestIntegrationGetProjectsAndSelf(t *testing.T) { + c := liveClient(t) + projects, err := c.GetProjects() + if err != nil { + t.Fatalf("GetProjects: %v", err) + } + if projects == nil { + t.Fatal("nil slice from GetProjects") + } + ctx := context.Background() + uid, err := c.SelfUserID(ctx) + if err != nil { + t.Fatalf("SelfUserID: %v", err) + } + if uid == "" { + t.Fatal("empty self user id") + } +} + +func TestIntegrationProjectAndTaskLifecycle(t *testing.T) { + c := liveClient(t) + t.Cleanup(func() { cleanupTestProjects(t, c) }) + + suffix := time.Now().UTC().Format("20060102-150405") + title := testProjectPrefix + suffix + project, err := c.CreateProject(NewProjectRequest{ + Title: title, + Description: "integration test from go-onlyoffice", + }) + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + if project.ID == nil { + t.Fatal("created project without id") + } + + updated, err := c.UpdateProject(ProjectUpdateRequest{ + ID: *project.ID, + Title: title + " (updated)", + Description: "updated", + ResponsibleID: *project.Responsible.ID, + }) + if err != nil { + t.Fatalf("UpdateProject: %v", err) + } + if updated.Title == nil || *updated.Title != title+" (updated)" { + t.Errorf("title not updated: %+v", updated.Title) + } + + start := Time(time.Now().AddDate(0, 0, -2)) + deadline := Time(time.Now().AddDate(0, 0, 2)) + task, err := c.CreateProjectTask(NewProjectTaskRequest{ + ProjectId: *project.ID, + Title: "integration parent task", + Description: "from go-onlyoffice integration suite", + StartDate: start, + Deadline: deadline, + Priority: int(TaskPriorityNormal), + }) + if err != nil { + t.Fatalf("CreateProjectTask: %v", err) + } + if task.ID == nil { + t.Fatal("created task without id") + } + + newStart := Time(time.Now().AddDate(0, 0, -14)) + newDeadline := Time(time.Now().AddDate(0, 0, 3)) + if _, err := c.UpdateProjectTask(ProjectTaskUpdateRequest{ + ID: *task.ID, + Title: "integration parent task (updated)", + Description: "updated", + StartDate: &newStart, + Deadline: &newDeadline, + }); err != nil { + t.Fatalf("UpdateProjectTask: %v", err) + } + + // Subtask uses the form-encoded helper; exercises httpx.postForm path. + ctx := context.Background() + sub, err := c.AddSubtask(ctx, strconv.Itoa(*task.ID), "integration subtask") + if err != nil { + t.Fatalf("AddSubtask: %v", err) + } + if _, ok := sub["id"]; !ok { + t.Errorf("subtask response missing id: %+v", sub) + } +} + +func TestIntegrationCalendarAndCRMRead(t *testing.T) { + c := liveClient(t) + ctx := context.Background() + start := time.Now().Format("2006-01-02") + end := time.Now().AddDate(0, 0, 14).Format("2006-01-02") + + if _, err := c.ListCalendars(ctx, start, end); err != nil { + t.Errorf("ListCalendars: %v", err) + } + if _, err := c.ListEvents(ctx, start, end); err != nil { + t.Errorf("ListEvents: %v", err) + } + if _, _, err := c.ListContacts(ctx, 5, 0, ""); err != nil { + t.Errorf("ListContacts: %v", err) + } + if _, _, err := c.ListOpportunities(ctx, 5, 0); err != nil { + t.Errorf("ListOpportunities: %v", err) + } + if _, err := c.ListDealStages(ctx); err != nil { + t.Errorf("ListDealStages: %v", err) + } +} + +// TestIntegrationDryRunList covers the tiniest project-tasks read that +// inventar-sync depends on; confirms GetTasks deserialises real responses. +func TestIntegrationListTasks(t *testing.T) { + c := liveClient(t) + defaults := GetEnvironmentDefaults() + // ProjectID default is "33"; skip the read-only list if the caller hasn't + // pointed us at a valid project (we don't know which projects exist). + if defaults.ProjectID == "" { + t.Skip("ONLYOFFICE_PROJECT_ID not configured") + } + pid, err := strconv.Atoi(defaults.ProjectID) + if err != nil || pid <= 0 { + t.Skipf("ONLYOFFICE_PROJECT_ID %q is not a positive int", defaults.ProjectID) + } + if _, err := c.GetTasks(NewProjectGetTasksRequest(pid)); err != nil { + t.Fatalf("GetTasks(%d): %v", pid, err) } } diff --git a/httpx_test.go b/httpx_test.go deleted file mode 100644 index fabf482..0000000 --- a/httpx_test.go +++ /dev/null @@ -1,270 +0,0 @@ -package onlyoffice - -// Unit tests for the form/multipart helpers and a few CRM/Calendar/Subtask -// methods. These do not require a real OnlyOffice server — they spin up a -// net/http/httptest.Server that emulates the expected endpoints. - -import ( - "context" - "encoding/json" - "io" - "mime" - "mime/multipart" - "net/http" - "net/http/httptest" - "os" - "strings" - "testing" - "time" -) - -// fakeServer wires a minimal OnlyOffice-like API: it always returns the same -// token on /api/2.0/authentication.json and dispatches remaining requests via -// the caller-supplied handler. -func fakeServer(t *testing.T, h http.HandlerFunc) *httptest.Server { - t.Helper() - mux := http.NewServeMux() - mux.HandleFunc("/api/2.0/authentication.json", func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"response":{"token":"TEST_TOKEN","expires":"2099-01-01T00:00:00Z"}}`) - }) - mux.HandleFunc("/", h) - return httptest.NewServer(mux) -} - -// newTestClient builds a Client pointed at the test server with a primed token -// so helpers skip the auth round-trip when the test does not exercise it. -func newTestClient(srv *httptest.Server) *Client { - c := NewClient(Credentials{Url: srv.URL, User: "u", Password: "p"}) - c.token = &Token{Value: "TEST_TOKEN", Expires: Time(time.Now().Add(time.Hour))} - return c -} - -func TestAddSubtaskFormEncoded(t *testing.T) { - var gotPath, gotAuth, gotCT, gotBody string - srv := fakeServer(t, func(w http.ResponseWriter, r *http.Request) { - gotPath = r.URL.Path - gotAuth = r.Header.Get("Authorization") - gotCT = r.Header.Get("Content-Type") - b, _ := io.ReadAll(r.Body) - gotBody = string(b) - w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"response":{"id":99,"title":"child"}}`) - }) - defer srv.Close() - c := newTestClient(srv) - - out, err := c.AddSubtask(context.Background(), "42", "child") - if err != nil { - t.Fatalf("AddSubtask: %v", err) - } - if gotPath != "/api/2.0/project/task/42.json" { - t.Errorf("path = %q", gotPath) - } - if gotAuth != "TEST_TOKEN" { - t.Errorf("auth header = %q", gotAuth) - } - if !strings.HasPrefix(gotCT, "application/x-www-form-urlencoded") { - t.Errorf("content-type = %q", gotCT) - } - if gotBody != "title=child" { - t.Errorf("body = %q", gotBody) - } - if fm, ok := out["title"].(string); !ok || fm != "child" { - t.Errorf("out.title = %v", out["title"]) - } -} - -func TestAddEventUsesDefaultCalendar(t *testing.T) { - var gotPath, gotBody string - srv := fakeServer(t, func(w http.ResponseWriter, r *http.Request) { - gotPath = r.URL.Path - b, _ := io.ReadAll(r.Body) - gotBody = string(b) - w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"response":[{"id":"e1","name":"hi"}]}`) - }) - defer srv.Close() - c := newTestClient(srv) - c.SetDefaults(Defaults{CalendarID: "7"}) - - ev, err := c.AddEvent(context.Background(), "", "hi", "2025-01-01T10:00:00Z", "2025-01-01T11:00:00Z", "desc", false) - if err != nil { - t.Fatalf("AddEvent: %v", err) - } - if gotPath != "/api/2.0/calendar/7/event.json" { - t.Errorf("path = %q", gotPath) - } - if !strings.Contains(gotBody, "name=hi") || !strings.Contains(gotBody, "isAllDayLong=false") { - t.Errorf("body = %q", gotBody) - } - if ev["id"] != "e1" { - t.Errorf("event id = %v", ev["id"]) - } -} - -func TestAddEventRequiresCalendar(t *testing.T) { - srv := fakeServer(t, func(w http.ResponseWriter, r *http.Request) { - t.Fatalf("server must not be hit without calendar id") - }) - defer srv.Close() - c := newTestClient(srv) - if _, err := c.AddEvent(context.Background(), "", "t", "s", "e", "", false); err == nil { - t.Fatal("expected error when no calendar id and no default") - } -} - -func TestListContactsTotal(t *testing.T) { - srv := fakeServer(t, func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/2.0/crm/contact/filter.json" { - t.Errorf("path = %q", r.URL.Path) - } - if r.URL.Query().Get("filterValue") != "acme" { - t.Errorf("filterValue = %q", r.URL.Query().Get("filterValue")) - } - w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"response":[{"id":1,"displayName":"ACME"}],"total":7}`) - }) - defer srv.Close() - c := newTestClient(srv) - - items, total, err := c.ListContacts(context.Background(), 50, 0, "acme") - if err != nil { - t.Fatalf("ListContacts: %v", err) - } - if total != 7 { - t.Errorf("total = %d", total) - } - if len(items) != 1 { - t.Fatalf("len(items) = %d", len(items)) - } -} - -func TestUploadOpportunityFileMultipart(t *testing.T) { - tmp := t.TempDir() + "/a.txt" - if err := writeFile(tmp, "hello"); err != nil { - t.Fatal(err) - } - var gotField, gotFilename, gotBody string - srv := fakeServer(t, func(w http.ResponseWriter, r *http.Request) { - ct := r.Header.Get("Content-Type") - if !strings.HasPrefix(ct, "multipart/form-data") { - t.Errorf("content-type = %q", ct) - } - _, params, err := mime.ParseMediaType(ct) - if err != nil { - t.Fatal(err) - } - mr := multipart.NewReader(r.Body, params["boundary"]) - part, err := mr.NextPart() - if err != nil { - t.Fatal(err) - } - gotField = part.FormName() - gotFilename = part.FileName() - b, _ := io.ReadAll(part) - gotBody = string(b) - w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"response":{"id":1,"title":"a.txt"}}`) - }) - defer srv.Close() - c := newTestClient(srv) - - out, err := c.UploadOpportunityFile(context.Background(), "123", tmp) - if err != nil { - t.Fatalf("UploadOpportunityFile: %v", err) - } - if gotField != "file" || gotFilename != "a.txt" || gotBody != "hello" { - t.Errorf("multipart = field=%q filename=%q body=%q", gotField, gotFilename, gotBody) - } - if out["title"] != "a.txt" { - t.Errorf("out.title = %v", out["title"]) - } -} - -func writeFile(path, content string) error { - return os.WriteFile(path, []byte(content), 0o600) -} - -func TestResponseFieldMissing(t *testing.T) { - _, err := responseField(json.RawMessage(`{"other":1}`), "response") - if err == nil { - t.Fatal("expected error on missing field") - } -} - -func TestAuthenticateContextUsesCacheWhenFresh(t *testing.T) { - var authHits int - mux := http.NewServeMux() - mux.HandleFunc("/api/2.0/authentication.json", func(w http.ResponseWriter, r *http.Request) { - authHits++ - w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"response":{"token":"FRESH_TOKEN","expires":"2099-01-01T00:00:00.0000000-00:00"}}`) - }) - srv := httptest.NewServer(mux) - defer srv.Close() - c := NewClient(Credentials{Url: srv.URL, User: "u", Password: "p"}) - - if err := c.AuthenticateContext(context.Background()); err != nil { - t.Fatalf("first AuthenticateContext: %v", err) - } - if authHits != 1 { - t.Errorf("expected 1 auth hit after first call, got %d", authHits) - } - if c.token == nil || c.token.Value != "FRESH_TOKEN" { - t.Errorf("token not cached: %+v", c.token) - } - if err := c.AuthenticateContext(context.Background()); err != nil { - t.Fatalf("second AuthenticateContext: %v", err) - } - if authHits != 1 { - t.Errorf("cache bypassed: expected 1 auth hit, got %d", authHits) - } -} - -func TestInvalidateTokenForcesReauth(t *testing.T) { - var authHits int - mux := http.NewServeMux() - mux.HandleFunc("/api/2.0/authentication.json", func(w http.ResponseWriter, r *http.Request) { - authHits++ - w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"response":{"token":"T","expires":"2099-01-01T00:00:00.0000000-00:00"}}`) - }) - srv := httptest.NewServer(mux) - defer srv.Close() - c := NewClient(Credentials{Url: srv.URL, User: "u", Password: "p"}) - - if err := c.AuthenticateContext(context.Background()); err != nil { - t.Fatalf("auth: %v", err) - } - c.InvalidateToken() - if c.token != nil { - t.Fatalf("token still cached after Invalidate: %+v", c.token) - } - if err := c.AuthenticateContext(context.Background()); err != nil { - t.Fatalf("re-auth: %v", err) - } - if authHits != 2 { - t.Errorf("expected 2 auth hits after invalidate, got %d", authHits) - } -} - -func TestAuthenticateContextRespectsCancellation(t *testing.T) { - mux := http.NewServeMux() - mux.HandleFunc("/api/2.0/authentication.json", func(w http.ResponseWriter, r *http.Request) { - select { - case <-r.Context().Done(): - return - case <-time.After(2 * time.Second): - _, _ = io.WriteString(w, `{"response":{"token":"T","expires":"2099-01-01T00:00:00.0000000-00:00"}}`) - } - }) - srv := httptest.NewServer(mux) - defer srv.Close() - c := NewClient(Credentials{Url: srv.URL, User: "u", Password: "p"}) - ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) - defer cancel() - if err := c.AuthenticateContext(ctx); err == nil { - t.Fatal("expected error on context timeout") - } -} diff --git a/internal/applications/applications.go b/internal/applications/applications.go index b90a402..34218e0 100644 --- a/internal/applications/applications.go +++ b/internal/applications/applications.go @@ -24,19 +24,19 @@ type RecruiterInfo struct { } type Data struct { - Path string - Folder string - Position string - Company string - Location string - Salary string - SalaryValue float64 - Contract string - Source string - Link string - Recruiter RecruiterInfo - Summary string - Documents []string + Path string + Folder string + Position string + Company string + Location string + Salary string + SalaryValue float64 + Contract string + Source string + Link string + Recruiter RecruiterInfo + Summary string + Documents []string } func Discover(base string) ([]string, error) { @@ -212,7 +212,9 @@ func buildSummary(app Data, text string) string { if m := regexp.MustCompile(`(?i)## Application Status\s*\n((?:[-*\[\]xX ].+\n?)+)`).FindStringSubmatch(text); len(m) > 1 { lines = append(lines, "", "Status:", strings.TrimSpace(m[1])) } - if m := regexp.MustCompile(`(?i)## (?:Fit Assessment|Match|Candidate Fit)\s*\n([\s\S]+?)(?=\n## |\z)`).FindStringSubmatch(text); len(m) > 1 { + // RE2 does not support lookahead; consume the trailing delimiter (\n## or EOS) + // with a non-capturing alternation — the captured group 1 stops right before it. + if m := regexp.MustCompile(`(?i)## (?:Fit Assessment|Match|Candidate Fit)\s*\n([\s\S]+?)(?:\n## |$)`).FindStringSubmatch(text); len(m) > 1 { ft := strings.TrimSpace(m[1]) if len(ft) > 500 { ft = ft[:500] + "..." diff --git a/internal/applications/parse_test.go b/internal/applications/parse_test.go index 0a4bed2..5bfa88e 100644 --- a/internal/applications/parse_test.go +++ b/internal/applications/parse_test.go @@ -20,3 +20,34 @@ func TestParseSalary(t *testing.T) { t.Fatalf("daily: %v", v) } } + +// TestBuildSummaryFitAssessment covers the RE2-safe replacement of a previously +// lookahead-based regex that would panic at MustCompile time on Go regexp. +func TestBuildSummaryFitAssessment(t *testing.T) { + app := Data{Position: "SRE", Company: "Acme", Folder: "/tmp"} + withNextSection := "## Fit Assessment\nGreat fit overall.\nStrong background.\n## Next section\nUnrelated." + summary := buildSummary(app, withNextSection) + if !contains(summary, "Fit Assessment:") { + t.Fatalf("missing header in summary:\n%s", summary) + } + if !contains(summary, "Great fit overall.") { + t.Fatalf("missing body in summary:\n%s", summary) + } + if contains(summary, "Unrelated.") { + t.Fatalf("bled into next section:\n%s", summary) + } + eof := "## Fit Assessment\nOnly content at EOF." + summary = buildSummary(app, eof) + if !contains(summary, "Only content at EOF.") { + t.Fatalf("EOF case missing body:\n%s", summary) + } +} + +func contains(haystack, needle string) bool { + for i := 0; i+len(needle) <= len(haystack); i++ { + if haystack[i:i+len(needle)] == needle { + return true + } + } + return false +} diff --git a/onlyoffice.go b/onlyoffice.go index eeb476f..d7e2580 100644 --- a/onlyoffice.go +++ b/onlyoffice.go @@ -3,15 +3,17 @@ package onlyoffice // OnlyOffice client package import ( + "bytes" "encoding/json" "fmt" - "github.com/google/go-querystring/query" "io" "net/http" "os" - regexp "regexp" + "regexp" "strings" "time" + + "github.com/google/go-querystring/query" ) // NewClient API @@ -295,7 +297,10 @@ type ProjectOwner struct { // String for Project to return title func (p Project) String() string { - return fmt.Sprintf(*p.Title) + if p.Title == nil { + return "" + } + return *p.Title } // Token OnlyOffice @@ -353,16 +358,16 @@ func (r Request) GetMethod() string { // Query the OnlyOffice API // - If request.Method is not set then it will default to GET -// - If request.Code is not nil then it will be marshaled to JSON -// - If request.Token is nil then it will get a token +// - If request.Body is not nil then it will be marshaled to JSON (unless already []byte/string) +// - If request.Token is nil then it will get (or reuse) a cached token // - If request.Token is not nil then it will be used as Authorization header // - If request.NoAuth is true then it will skip automatic authentication -func (c *Client) Query(request Request, result interface{}) (err error) { - var url = fmt.Sprintf("%s%s", c.credentials.Url, request.Uri) - var rdr io.Reader = nil - var jsonRequestBody string +// +// The Debug field is preserved for backwards compatibility; it no longer +// changes behaviour — both branches used to unmarshal into the same target. +func (c *Client) Query(request Request, result interface{}) error { + url := c.credentials.Url + request.Uri - // Add query parameters if available if request.Params != nil { v, err := query.Values(request.Params) if err != nil { @@ -371,78 +376,62 @@ func (c *Client) Query(request Request, result interface{}) (err error) { url = fmt.Sprintf("%s?%s", url, v.Encode()) } - // Create request - if request.Body != nil { - // Check request body if type is not []byte or string the marshal it to JSON - switch request.Body.(type) { - case []byte: - jsonRequestBody = string(request.Body.([]byte)) - rdr = strings.NewReader(string(request.Body.([]byte))) - case string: - jsonRequestBody = request.Body.(string) - rdr = strings.NewReader(request.Body.(string)) - default: - // Marshal to JSON - b, err := json.Marshal(request.Body) - if err != nil { - return fmt.Errorf("failed to marshal request body: %v", err) - } - jsonRequestBody = string(b) - rdr = strings.NewReader(string(b)) - } + rdr, err := requestBodyReader(request.Body) + if err != nil { + return err } req, err := http.NewRequest(request.GetMethod(), url, rdr) if err != nil { return err } - req.Header.Set("Accept", "application/json") req.Header.Set("Content-Type", "application/json") req.Header.Set("Pragma", "no-cache") - // Get token if not set? if !request.NoAuth { - // Get token if not set or expired - if c.token == nil || time.Time(c.token.Expires).Before(time.Now()) { - c.token, err = c.Auth(c.credentials) - if err != nil { - return fmt.Errorf("failed to authenticate: %v", err) - } + if err := c.ensureToken(); err != nil { + return fmt.Errorf("failed to authenticate: %w", err) } } - - // Set token if available - if request.Token != nil { + switch { + case request.Token != nil: req.Header.Set("Authorization", *request.Token) - } else { - // Set token from a client if available - if c.token != nil { - req.Header.Set("Authorization", c.token.Value) - } + case c.token != nil: + req.Header.Set("Authorization", c.token.Value) } resp, err := c.client.Do(req) if err != nil { - return fmt.Errorf("failed to send request: %v", err) + return fmt.Errorf("failed to send request: %w", err) } defer resp.Body.Close() if result == nil { return nil } - if request.Debug { - // Rewind reader - _ = jsonRequestBody - var buf = new(strings.Builder) - io.Copy(buf, resp.Body) - js := buf.String() - return json.Unmarshal([]byte(js), result) - } else { - // Unmarshal response using reader - return json.NewDecoder(resp.Body).Decode(result) - } + return json.NewDecoder(resp.Body).Decode(result) +} +// requestBodyReader normalises Query() body input into an io.Reader. +// []byte and string are passed through verbatim; everything else is marshalled +// to JSON. Returns (nil, nil) for a nil body. +func requestBodyReader(body any) (io.Reader, error) { + if body == nil { + return nil, nil + } + switch b := body.(type) { + case []byte: + return bytes.NewReader(b), nil + case string: + return strings.NewReader(b), nil + default: + j, err := json.Marshal(body) + if err != nil { + return nil, fmt.Errorf("failed to marshal request body: %w", err) + } + return bytes.NewReader(j), nil + } } // Auth to authenticate by getting a token using credentials @@ -489,7 +478,7 @@ func (c *Client) GetProjects() (list Projects, err error) { func (c *Client) GetProjectMilestones(project *Project) ([]*Milestone, error) { var list []*Milestone - err := c.Query(Request{Uri: fmt.Sprintf(`/api/2.0/project/%d/milestone`, *project.ID), Debug: false}, + err := c.Query(Request{Uri: fmt.Sprintf(`/api/2.0/project/%d/milestone`, *project.ID)}, &struct { MetaResponse `json:",inline"` //Response *[]*Milestone @@ -656,9 +645,7 @@ func (c *Client) UpdateProjectTask(req ProjectTaskUpdateRequest) (task *Task, er Uri: fmt.Sprintf("/api/2.0/project/task/%d.json", req.ID), Method: "PUT", Body: req, - Debug: true, }, &struct { - //MetaResponse `json:",inline"` Response *Task `json:"response"` }{task}) } @@ -692,10 +679,8 @@ func (c *Client) GetTasks(req ProjectGetTasksRequest) (tasks []*Task, err error) Request{ Uri: "/api/2.0/project/task/filter.json", Params: req, - Debug: true, }, &struct { - //MetaResponse `json:",inline"` Response *[]*Task `json:"response"` }{&tasks}) } diff --git a/unit_test.go b/unit_test.go new file mode 100644 index 0000000..6640cbe --- /dev/null +++ b/unit_test.go @@ -0,0 +1,133 @@ +package onlyoffice + +// Pure unit tests — no network, no fake vendor HTTP servers. +// Protocol-level behaviour is covered by *_integration_test.go (build-tagged). + +import ( + "context" + "encoding/json" + "testing" + "time" +) + +func TestResponseFieldMissing(t *testing.T) { + if _, err := responseField(json.RawMessage(`{"other":1}`), "response"); err == nil { + t.Fatal("expected error on missing field") + } +} + +func TestResponseFieldPresent(t *testing.T) { + raw, err := responseField(json.RawMessage(`{"response":[1,2,3],"other":9}`), "response") + if err != nil { + t.Fatal(err) + } + if string(raw) != "[1,2,3]" { + t.Errorf("got %s", string(raw)) + } +} + +func TestRequestBodyReaderNil(t *testing.T) { + r, err := requestBodyReader(nil) + if err != nil { + t.Fatal(err) + } + if r != nil { + t.Errorf("expected nil reader for nil body, got %T", r) + } +} + +func TestRequestBodyReaderBytes(t *testing.T) { + r, err := requestBodyReader([]byte(`{"k":1}`)) + if err != nil { + t.Fatal(err) + } + if r == nil { + t.Fatal("nil reader") + } +} + +func TestRequestBodyReaderStruct(t *testing.T) { + type payload struct { + Name string `json:"name"` + } + r, err := requestBodyReader(payload{Name: "x"}) + if err != nil { + t.Fatal(err) + } + buf := make([]byte, 64) + n, _ := r.Read(buf) + if string(buf[:n]) != `{"name":"x"}` { + t.Errorf("got %q", string(buf[:n])) + } +} + +func TestProjectStringNilSafe(t *testing.T) { + var p Project + if got := p.String(); got != "" { + t.Errorf("nil Title should yield empty string, got %q", got) + } + title := "my project" + p.Title = &title + if got := p.String(); got != "my project" { + t.Errorf("got %q", got) + } + titleWithPercent := "100% coverage" + p.Title = &titleWithPercent + if got := p.String(); got != "100% coverage" { + t.Errorf("Sprintf format-string regression: got %q", got) + } +} + +func TestAuthenticateContextRespectsCancellation(t *testing.T) { + // Point the client at a routable-but-unresponsive endpoint (TEST-NET-1 + // per RFC 5737) and cancel the context almost immediately. The test + // verifies ctx plumbing, not OnlyOffice protocol — no vendor mock. + c := NewClient(Credentials{Url: "http://192.0.2.1:9", User: "u", Password: "p"}) + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Millisecond) + defer cancel() + if err := c.AuthenticateContext(ctx); err == nil { + t.Fatal("expected error on context timeout against unreachable endpoint") + } +} + +func TestInvalidateTokenIsIdempotent(t *testing.T) { + c := NewClient(Credentials{Url: "http://example.invalid", User: "u", Password: "p"}) + c.InvalidateToken() + c.InvalidateToken() + if c.token != nil { + t.Fatal("token should remain nil after double invalidate") + } +} + +func TestGetEnvironmentCredentialsAliases(t *testing.T) { + t.Setenv("ONLYOFFICE_URL", "") + t.Setenv("ONLYOFFICE_HOST", "https://example/") + t.Setenv("ONLYOFFICE_USER", "") + t.Setenv("ONLYOFFICE_NAME", "alice") + t.Setenv("ONLYOFFICE_PASS", "") + t.Setenv("ONLYOFFICE_PASSWORD", "s3cret") + c := GetEnvironmentCredentials() + if c.Url != "https://example" { + t.Errorf("Url alias not applied / trailing slash not trimmed: %q", c.Url) + } + if c.User != "alice" { + t.Errorf("User alias not applied: %q", c.User) + } + if c.Password != "s3cret" { + t.Errorf("Password alias not applied: %q", c.Password) + } +} + +func TestGetEnvironmentDefaultsFallbacks(t *testing.T) { + t.Setenv("ONLYOFFICE_CALENDAR_ID", "") + t.Setenv("ONLYOFFICE_PROJECT_ID", "") + t.Setenv("ONLYOFFICE_CALENDAR_PROJECT_ID", "") + d := GetEnvironmentDefaults() + if d.CalendarID != "1" || d.ProjectID != "33" { + t.Errorf("defaults: %+v", d) + } + t.Setenv("ONLYOFFICE_CALENDAR_PROJECT_ID", "7") + if got := GetEnvironmentDefaults().ProjectID; got != "7" { + t.Errorf("CalendarProjectId alias: %q", got) + } +}