Fix/model json unmarshaling

[ilopezluna]

Go's standard json.Unmarshal cannot deserialize JSON directly into an interface type (types.ModelConfig). This caused issues when deserializing Model objects received over HTTP (e.g., from the scheduler API).

docker model ls
Failed to list models: failed to unmarshal response body: json: cannot unmarshal object into Go struct field Model.config of type types.ModelConfig

This PR adds a custom UnmarshalJSON on Model using the alias pattern and json.RawMessage to decode the config field into the supported concrete config type.

docker model ls
MODEL NAME                              PARAMETERS  QUANTIZATION    ARCHITECTURE  MODEL ID      CREATED       CONTEXT  SIZE       
example                                 162.97M     MOSTLY_Q8_0     llama         4b9cbb0015eb  46 hours ago  8192     165.24MiB  
gpt-oss                                 20.91 B     MOSTLY_Q4_K_M   gpt-oss       9398339cb0d3  2 months ago           11.04 GiB  
huggingface.co/unsloth/qwen3-0.6b-gguf  596M                        qwen3         fde891dc5f5a  8 months ago           397M       
qwen3:0.6B-F16                          751.63 M    F16             qwen3         8cd0c2b3ebf0  8 months ago           1.40 GiB   
qwen3-coder                             30.53 B     IQ2_XXS/Q4_K_M  qwen3moe      6cd9fcd122e1  3 months ago           16.45 GiB  

[sourcery-ai]

Hey - I've found 4 issues, and left some high level feedback:

• In UnmarshalJSON, when config is missing or explicitly null you return early without touching m.Config; if a Model instance is reused for multiple unmarshals this can leave stale config data—consider explicitly setting m.Config = nil in that branch to make the behavior robust to reuse.
• The current implementation always decodes config into types.Config even though Config is an interface (types.ModelConfig); if you plan to support multiple concrete ModelConfig implementations, you may want to introduce a discriminator or format-based switch so UnmarshalJSON can choose the appropriate concrete type instead of hardcoding types.Config.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `UnmarshalJSON`, when `config` is missing or explicitly `null` you return early without touching `m.Config`; if a `Model` instance is reused for multiple unmarshals this can leave stale config data—consider explicitly setting `m.Config = nil` in that branch to make the behavior robust to reuse.
- The current implementation always decodes `config` into `types.Config` even though `Config` is an interface (`types.ModelConfig`); if you plan to support multiple concrete `ModelConfig` implementations, you may want to introduce a discriminator or format-based switch so `UnmarshalJSON` can choose the appropriate concrete type instead of hardcoding `types.Config`.

## Individual Comments

### Comment 1
<location> `pkg/inference/models/api.go:125` </location>
<code_context>
+// future extension to support multiple ModelConfig implementations.
+func (m *Model) UnmarshalJSON(data []byte) error {
+	type Alias Model
+	aux := struct {
+		*Alias
+		Config json.RawMessage `json:"config"`
</code_context>

<issue_to_address>
**suggestion:** Shadowing the `config` field via the embedded alias is subtle and may be confusing; consider making the intent more explicit.

The `aux` struct embeds `*Alias` (which has `Config types.ModelConfig 	`json:"config"``) and also declares `Config json.RawMessage` with the same JSON tag. Although the explicit field will be used for unmarshaling, this shadowing is subtle and may confuse future changes. Consider using an alias type without `Config`, or renaming/tag-adjusting the raw-message field (e.g., omit the tag on the embedded alias or use a different Go name) to avoid ambiguity.

Suggested implementation:

```golang
 // UnmarshalJSON implements custom JSON unmarshaling for Model.
 // This is necessary because Config is an interface type (types.ModelConfig),
 // and Go's standard JSON decoder cannot unmarshal directly into an interface.
 // We use json.RawMessage to defer parsing of the config field, allowing for
 // future extension to support multiple ModelConfig implementations.
 func (m *Model) UnmarshalJSON(data []byte) error {
 	// First, unmarshal into a raw map so we can peel off the "config" field
 	// without relying on shadowing between fields with the same JSON tag.
 	var raw map[string]json.RawMessage
 	if err := json.Unmarshal(data, &raw); err != nil {
 		return err
 	}

 	// Extract and remove the raw config from the payload.
 	rawConfig, hasConfig := raw["config"]
 	if hasConfig {
 		delete(raw, "config")
 	}

 	// Unmarshal the remaining fields into the Model via an alias type
 	// that does not have its own UnmarshalJSON, avoiding recursion.
 	type Alias Model
 	if err := json.Unmarshal(mustMarshal(raw), (*Alias)(m)); err != nil {
 		return err
 	}

 	// If there is no config field (or it's explicitly null), leave Config as-is.
 	if !hasConfig || len(rawConfig) == 0 || bytes.Equal(rawConfig, []byte("null")) {
 		return nil
 	}

 	// At this point, rawConfig holds the raw JSON for the config field.
 	// The actual decoding into a concrete types.ModelConfig implementation
 	// is handled below (existing logic retained).
 	configBytes := rawConfig

```

1. The helper `mustMarshal` used above needs to be implemented in this file (or replaced with your own error-handling wrapper). A simple implementation could be:
   ```go
   func mustMarshal(v any) []byte {
       b, err := json.Marshal(v)
       if err != nil {
           panic(err) // or handle according to your error strategy
       }
       return b
   }
   ```
   Alternatively, inline `json.Marshal(raw)` with proper error handling instead of using `mustMarshal`.

2. Wherever the original code referenced `aux.Config` after the shown snippet, replace those references with `configBytes` (the `[]byte` containing the raw config JSON). For example, if you previously had:
   ```go
   if err := someDecodeFunc(aux.Config, &m.Config); err != nil {
       return err
   }
   ```
   update it to:
   ```go
   if err := someDecodeFunc(configBytes, &m.Config); err != nil {
       return err
   }
   ```

3. If there is additional logic after the `if len(aux.Config) == 0 || ...` block (e.g., switching on a type field to pick a concrete `types.ModelConfig` implementation), keep that logic unchanged, but use `configBytes` as the source raw JSON instead of `aux.Config`.

4. If you prefer not to introduce `mustMarshal`, you can replace:
   ```go
   if err := json.Unmarshal(mustMarshal(raw), (*Alias)(m)); err != nil {
   ```
   with:
   ```go
   buf, err := json.Marshal(raw)
   if err != nil {
       return err
   }
   if err := json.Unmarshal(buf, (*Alias)(m)); err != nil {
       return err
   }
   ```
</issue_to_address>

### Comment 2
<location> `pkg/inference/models/api.go:136-137` </location>
<code_context>
+		return err
+	}
+
+	if len(aux.Config) == 0 || bytes.Equal(aux.Config, []byte("null")) {
+		return nil
+	}
+
</code_context>

<issue_to_address>
**issue (bug_risk):** When `config` is null or missing, `m.Config` is left untouched, which can leak a previous value.

Because this branch returns without updating `m.Config`, reusing the same `Model` across unmarshals means a payload with `"config": null` (or no `config` field) will silently retain the old value. To avoid this surprising state leak, set `m.Config = nil` before returning so the struct matches the input JSON.
</issue_to_address>

### Comment 3
<location> `pkg/inference/models/api_test.go:12` </location>
<code_context>
+	"github.com/stretchr/testify/require"
+)
+
+func TestModelUnmarshalJSON(t *testing.T) {
+	tests := []struct {
+		name     string
</code_context>

<issue_to_address>
**suggestion (testing):** Add test cases for missing and null `config` to cover the early-return paths in `UnmarshalJSON`.

There’s explicit handling for both `len(aux.Config) == 0` and `config: null`, but no tests cover these branches. Please add two cases to `TestModelUnmarshalJSON`: one where `config` is omitted, and one with `"config": null`. Both should verify that `model.Config` stays `nil` while the other fields are unmarshaled correctly, so the custom behavior for missing/null config is covered and protected from regressions.

Suggested implementation:

```golang
	"github.com/docker/model-runner/pkg/distribution/types"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

func TestModelUnmarshalJSON_MissingAndNullConfig(t *testing.T) {
	tests := []struct {
		name     string
		jsonData string
	}{
		{
			name: "missing config field",
			jsonData: `{
				"id": "sha256:abc123",
				"tags": ["ai/smollm2:latest"],
				"created": 1704067200
			}`,
		},
		{
			name: "explicit null config field",
			jsonData: `{
				"id": "sha256:abc123",
				"tags": ["ai/smollm2:latest"],
				"created": 1704067200,
				"config": null
			}`,
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			var model Model
			err := json.Unmarshal([]byte(tt.jsonData), &model)
			require.NoError(t, err)

			// config should remain nil for both missing and null cases
			assert.Nil(t, model.Config)

			// other fields should still be populated correctly
			assert.Equal(t, "sha256:abc123", model.ID)
			assert.Equal(t, []string{"ai/smollm2:latest"}, model.Tags)
			assert.Equal(t, int64(1704067200), model.Created)
		})
	}
}

func TestModelUnmarshalJSON(t *testing.T) {

```

Depending on the actual definition of `Model`, you might need to adjust:
1. Field names/types in the assertions:
   - If `Created` is a `time.Time`, replace `assert.Equal(t, int64(1704067200), model.Created)` with an assertion that compares against `time.Unix(1704067200, 0)` (and ensure `time` is imported).
   - If the `ID` or `Tags` fields use different names or types, update the assertions accordingly.
2. The `Config` field type: the test assumes `model.Config` is a pointer (or interface) that is `nil` when unset. If it's a value type, change the assertion to match the zero value semantics you expect for missing/null config.
These adjustments will align the new tests with your actual `Model` struct while still covering the missing and null `config` branches.
</issue_to_address>

### Comment 4
<location> `pkg/inference/models/api_test.go:238` </location>
<code_context>
+	assert.Equal(t, *originalConfig.ContextSize, *unmarshaledConfig.ContextSize)
+}
+
+func TestModelUnmarshalJSONInvalidData(t *testing.T) {
+	tests := []struct {
+		name     string
</code_context>

<issue_to_address>
**suggestion (testing):** Extend invalid-data tests to cover malformed `config` values so the custom config unmarshaling path is exercised.

Current invalid-data cases only cover top-level JSON issues and type mismatches for `id` and `tags`, which are handled by alias decoding. Please add cases where `config` has an invalid shape (e.g. `{"id": "test", "config": "not-an-object"}` or `{"id": "test", "config": [1,2,3]}`) to ensure errors from `json.Unmarshal(aux.Config, &cfg)` are triggered and correctly surfaced by `Model.UnmarshalJSON`. This will exercise the custom unmarshaling logic, not just the alias path.

Suggested implementation:

```golang
func TestModelUnmarshalJSONInvalidData(t *testing.T) {
	tests := []struct {
		name        string
		jsonData    string
		errorSubstr string
	}{
		{
			name: "invalid top-level JSON",
			jsonData: `{
				"id": "test",
				"tags": ["ai/test:latest",]
			}`,
			errorSubstr: "invalid character",
		},
		{
			name: "invalid id type",
			jsonData: `{
				"id": 123,
				"tags": ["ai/test:latest"]
			}`,
			errorSubstr: "json: cannot unmarshal",
		},
		{
			name: "invalid tags type",
			jsonData: `{
				"id": "test",
				"tags": "not-a-list"
			}`,
			errorSubstr: "json: cannot unmarshal",
		},
		{
			name: "config is string (invalid shape)",
			jsonData: `{
				"id": "test",
				"config": "not-an-object"
			}`,
			errorSubstr: "json: cannot unmarshal",
		},
		{
			name: "config is array (invalid shape)",
			jsonData: `{
				"id": "test",
				"config": [1, 2, 3]
			}`,
			errorSubstr: "json: cannot unmarshal",
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			var m Model
			err := json.Unmarshal([]byte(tt.jsonData), &m)
			require.Error(t, err)
			if tt.errorSubstr != "" {
				assert.Contains(t, err.Error(), tt.errorSubstr)
			}
		})
	}

```

- If `TestModelUnmarshalJSONInvalidData` already contains some table entries, merge those existing entries into the `tests` slice above instead of replacing them, preserving the existing invalid `id`/`tags` coverage.
- Ensure `encoding/json` is imported as `json` and `testify/assert` and `testify/require` are imported; they appear to be used elsewhere in this file already.
- If your existing invalid test cases assert on concrete error values instead of substrings, adjust the `errorSubstr` checks to match your existing convention (e.g., compare to `ErrInvalidModel` or similar).
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[doringeman]

Fair!