accept both factset shapes and skip undecodable facts

Real PuppetDB answers /factsets/<certname> with an array; the fallback decoded a bare object, so it only worked against pdbmux's own divergent response. - decode an array of factsets, picking the requested certname - keep accepting a single bare factset object - drop and report a fact record that will not decode instead of failing the whole fetch - fail only when nothing in the response decodes
2026-09-12 17:19:28 +10:00
parent 6e4ee54203
commit 7f2c6f16c8
2 changed files with 184 additions and 12 deletions
@@ -4,6 +4,7 @@
 package pdbmux

 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -22,6 +23,10 @@ var ErrNodeNotFound = errors.New("node not found in pdbmux")
 type Client struct {
 	BaseURL string
 	HTTP    *http.Client
+
+	// Warn, when set, receives a message for every fact record that could not
+	// be decoded and was therefore dropped.
+	Warn func(string)
 }

 // New returns a client for baseURL.
@@ -38,14 +43,16 @@ type factRecord struct {
 }

 type factset struct {
-	Facts struct {
+	Certname string `json:"certname"`
+	Facts    struct {
 		Data []factRecord `json:"data"`
 	} `json:"facts"`
 }

 // Facts returns the node's facts as the values map a catalog request needs.
 // It reads /pdb/query/v4/nodes/<certname>/facts and falls back to
-// /pdb/query/v4/factsets/<certname>, which returns the same data nested.
+// /pdb/query/v4/factsets/<certname>, which returns the same data nested in
+// either an array of factsets or a single bare factset object.
 func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, error) {
 	body, status, err := c.get(ctx, "/pdb/query/v4/nodes/"+url.PathEscape(certname)+"/facts")
 	if err != nil {
@@ -57,7 +64,7 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er
 			return nil, fmt.Errorf("parsing facts for %s: %w", certname, err)
 		}
 		if len(records) > 0 {
-			return fold(records)
+			return c.fold(certname, records)
 		}
 	} else if status != http.StatusNotFound {
 		return nil, fmt.Errorf("pdbmux returned %d for %s facts: %s", status, certname, snippet(body))
@@ -74,14 +81,44 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er
 		return nil, fmt.Errorf("pdbmux returned %d for %s factset: %s", status, certname, snippet(body))
 	}

-	var fs factset
-	if err := json.Unmarshal(body, &fs); err != nil {
+	records, err := decodeFactsets(body, certname)
+	if err != nil {
 		return nil, fmt.Errorf("parsing factset for %s: %w", certname, err)
 	}
-	if len(fs.Facts.Data) == 0 {
+	if len(records) == 0 {
 		return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname)
 	}
-	return fold(fs.Facts.Data)
+	return c.fold(certname, records)
+}
+
+// decodeFactsets reads the two shapes /factsets/<certname> comes back as: real
+// PuppetDB returns an array of factsets, pdbmux currently returns a single
+// bare object.
+func decodeFactsets(body []byte, certname string) ([]factRecord, error) {
+	if bytes.HasPrefix(bytes.TrimLeft(body, " \t\r\n"), []byte("[")) {
+		var sets []factset
+		if err := json.Unmarshal(body, &sets); err != nil {
+			return nil, err
+		}
+		for _, fs := range sets {
+			if fs.Certname == certname {
+				return fs.Facts.Data, nil
+			}
+		}
+		if len(sets) == 1 {
+			return sets[0].Facts.Data, nil
+		}
+		if len(sets) == 0 {
+			return nil, nil
+		}
+		return nil, fmt.Errorf("%d factsets returned, none for the requested certname", len(sets))
+	}
+
+	var fs factset
+	if err := json.Unmarshal(body, &fs); err != nil {
+		return nil, err
+	}
+	return fs.Facts.Data, nil
 }

 func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) {
@@ -105,19 +142,30 @@ func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) {
 }

 // fold turns fact records into the flat name -> value map the catalog API
-// wants. Structured facts keep their nested shape.
-func fold(records []factRecord) (map[string]any, error) {
+// wants. Structured facts keep their nested shape. An undecodable record is
+// dropped and reported: one bad fact must not cost the whole set. A response
+// in which nothing decodes is a failed fetch, not an empty fact set.
+func (c *Client) fold(certname string, records []factRecord) (map[string]any, error) {
 	values := make(map[string]any, len(records))
+	var skipped []string
 	for _, r := range records {
 		if r.Name == "" {
 			continue
 		}
 		var v any
-		if err := json.Unmarshal(r.Value, &v); err != nil {
-			return nil, fmt.Errorf("parsing fact %q: %w", r.Name, err)
+		if len(r.Value) == 0 || json.Unmarshal(r.Value, &v) != nil {
+			skipped = append(skipped, r.Name)
+			continue
 		}
 		values[r.Name] = v
 	}
+	if len(values) == 0 {
+		return nil, fmt.Errorf("no usable facts for %s: all %d records were undecodable", certname, len(records))
+	}
+	if len(skipped) > 0 && c.Warn != nil {
+		c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) for %s: %s",
+			len(skipped), certname, strings.Join(skipped, ", ")))
+	}
 	return values, nil
 }

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 )
@@ -15,7 +16,9 @@ const nodeFactsBody = `[
  {"certname":"host.example.net","environment":"develop","name":"processorcount","value":4}
 ]`

-const factsetBody = `{
+// The shape real PuppetDB returns: /factsets/<certname> is a query endpoint
+// and answers with an array.
+const factsetBody = `[{
  "certname":"host.example.net",
  "environment":"develop",
  "timestamp":"2026-09-12T05:26:11.807Z",
@@ -24,6 +27,17 @@ const factsetBody = `{
    {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}},
    {"name":"processorcount","value":4}
  ]}
+}]`
+
+// The shape pdbmux itself currently returns for the same route.
+const bareFactsetBody = `{
+  "certname":"host.example.net",
+  "environment":"develop",
+  "facts":{"data":[
+    {"name":"id","value":"root"},
+    {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}},
+    {"name":"processorcount","value":4}
+  ]}
 }`

 func assertFacts(t *testing.T, facts map[string]any) {
@@ -157,3 +171,113 @@ func TestFactsContextCancelled(t *testing.T) {
 		t.Fatal("expected a timeout error")
 	}
 }
+
+// factsetServer answers the nodes route with a 404 and the factsets route with
+// body, which is the path the fallback takes.
+func factsetServer(t *testing.T, body string) *httptest.Server {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/pdb/query/v4/factsets/host.example.net" {
+			_, _ = w.Write([]byte(body))
+			return
+		}
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = w.Write([]byte(`{"error":"No information is known about node host.example.net"}`))
+	}))
+	t.Cleanup(srv.Close)
+	return srv
+}
+
+func TestFactsFallsBackToBareFactsetObject(t *testing.T) {
+	srv := factsetServer(t, bareFactsetBody)
+
+	facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+	if err != nil {
+		t.Fatalf("Facts: %v", err)
+	}
+	assertFacts(t, facts)
+}
+
+func TestFactsPicksTheRequestedCertnameFromAFactsetArray(t *testing.T) {
+	srv := factsetServer(t, `[
+	  {"certname":"other.example.net","facts":{"data":[{"name":"id","value":"nobody"}]}},
+	  {"certname":"host.example.net","facts":{"data":[
+	    {"name":"id","value":"root"},
+	    {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}},
+	    {"name":"processorcount","value":4}
+	  ]}}
+	]`)
+
+	facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+	if err != nil {
+		t.Fatalf("Facts: %v", err)
+	}
+	assertFacts(t, facts)
+}
+
+func TestFactsRejectsAFactsetArrayWithNoMatch(t *testing.T) {
+	srv := factsetServer(t, `[
+	  {"certname":"a.example.net","facts":{"data":[{"name":"id","value":"a"}]}},
+	  {"certname":"b.example.net","facts":{"data":[{"name":"id","value":"b"}]}}
+	]`)
+
+	_, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+	if err == nil {
+		t.Fatal("expected an error when no factset matches the certname")
+	}
+	if !strings.Contains(err.Error(), "none for the requested certname") {
+		t.Errorf("unhelpful error: %v", err)
+	}
+}
+
+func TestFactsEmptyFactsetArrayIsAMissingNode(t *testing.T) {
+	srv := factsetServer(t, `[]`)
+
+	_, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+	if !errors.Is(err, ErrNodeNotFound) {
+		t.Fatalf("err = %v, want ErrNodeNotFound", err)
+	}
+}
+
+func TestFactsSkipsUndecodableRecords(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(`[
+		  {"name":"id","value":"root"},
+		  {"name":"broken"},
+		  {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}},
+		  {"name":"processorcount","value":4}
+		]`))
+	}))
+	defer srv.Close()
+
+	var warnings []string
+	c := New(srv.URL, 5*time.Second)
+	c.Warn = func(msg string) { warnings = append(warnings, msg) }
+
+	facts, err := c.Facts(context.Background(), "host.example.net")
+	if err != nil {
+		t.Fatalf("one bad fact must not fail the fetch: %v", err)
+	}
+	assertFacts(t, facts)
+	if _, ok := facts["broken"]; ok {
+		t.Error("a valueless fact must be dropped, not stored")
+	}
+	if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") {
+		t.Errorf("the skipped fact must be reported, got %v", warnings)
+	}
+}
+
+func TestFactsWithNothingDecodableIsAnError(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(`[{"name":"a"},{"name":"b"}]`))
+	}))
+	defer srv.Close()
+
+	_, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+	if err == nil {
+		t.Fatal("a fact set in which nothing decodes must not pass as empty")
+	}
+	if !strings.Contains(err.Error(), "no usable facts") {
+		t.Errorf("unhelpful error: %v", err)
+	}
+}