Repository: bacalhau-project/bacalhau (language: python)

Issue:
Better errors on job version
```
❯ bacalhau job run job.yaml
the job provided is invalid: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go struct field Job.Version of type uint64
```

This error is very hard to understand. What should the version be? And why would we tell someone what unmarshalling is?


Discussion / hints:
the job version is auto generated and updated by the requester node and should be ignored if it was provided by the user. This is a bug and should be handled better
> cannot unmarshal string into Go struct field Job.Version of type uint64

@aronchick can you share the `job.yaml` file you used? And in the future for errors like this please always share the corresponding file :smile: 

> the job version is auto generated and updated by the requester node and should be ignored if it was provided by the user. This is a bug and should be handled better

Hummm, on the other hand I think there is value to alerting users they are providing a field that has no affect/shouldn't be specified - else they may continue to do it and not realize its a noop, basically a waste of their time and increased cogitative  overhead. 
Better, we should exclude fields used by the protocol/internally from the data users submit
fair enough. I think the benefit I was looking for is allowing users to submit a similar version of a job by doing something like `bacalhau job describe --output yaml | bacalhau job run`
Agreed - but I think it's perfectly valid to just ignore and alert the user as a warning "hey we ignored this field"
I like the warning approach instead of failing or silently ignoring the fields
I've created an issue to track separating user submission from system representation here: https://github.com/bacalhau-project/bacalhau/issues/3983
@frrist lets just ignore system defined fields such as `version` for now
One way to reproduce this error is with the following job spec:
```yaml
Name: A Simple Docker Job
Type: batch
Version: "10"
Count: 1
Tasks:
  - Name: My main task
    Engine:
      Type: docker
      Params:
        Image: ubuntu:latest
        Entrypoint:
          - /bin/bash
        Parameters:
          - -c
          - echo Hello Bacalhau!
```
The key thing here is that `Version` is of type `string` which doesn't decode into the expected `int64` type of the job structure. However, the same spec with `Version` as an int runs just fine; the field is ignored. Similar behavior applies to `State`, `Revision`, `CreateTime` and `ModifyTime`. 
The main problem here is that we are decoding the job file provided by users into the `models.Job` type before performing validation. If this decoding fails we get (obtuse) validation errors from the marshaller, rather than the validation code.

I think there are several ways we can address this:
1. Validate the input from users against a [schema](https://github.com/bacalhau-project/bacalhau/pull/4074) before unmarshalling into a job.
2. Marshal the input from users into a `map[string]interface` and make assertions on the keys present e.g.:
```go
func PreValidate(jobSpec map[string]interface{}) error {
	disallowedKeys := map[string]struct{}{
		"id":         {},
		"state":      {},
		"version":    {},
		"revision":   {},
		"createtime": {},
		"modifytime": {},
	}

	var mErr error
	for key := range jobSpec {
		specKey := strings.ToLower(key)
		if _, found := disallowedKeys[specKey]; found {
			mErr = errors.Join(mErr, fmt.Errorf("%s field not allowed", key))
		}
	}
	return mErr
}
```
3. Write a custom marshallers for the job type

I prefer option 1 since 3 is a lot of work to maintain, and 2 won't catch other issues, such as when fields like `Count` are mistyped:
```yaml
Name: A Simple Docker Job
Type: batch
Count: "1"
Tasks:
  - Name: My main task
    Engine:
      Type: docker
      Params:
        Image: ubuntu:latest
        Entrypoint:
          - /bin/bash
        Parameters:
          - -c
          - echo Hello Bacalhau!
```
calling `bacalhau job run` with the above spec produces the error message:
`the job provided is invalid: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go struct field Job.Count of type int` which option 2 won't be able to catch.

@wdbaruni how would you like us to address this issue?

Edit: I suppose another option would be a separate job type containing only the valid fields settable by users, similar to what was proposed in https://github.com/bacalhau-project/bacalhau/issues/3983. However that still won't solve all the issues around mistyping of required fields, such as the `Count` example described above
Since this is about providing an invalid job spec and not about piping `bacalhau job describe <id> | bacalhau job run`, then it is not a problem about failing to ignore system defined fields as you were able to reproduce the issue with `Count` field.

The main problem is how can we provide better errors when failing to unmarshal the object. I think we are over wrapping errors today as the message `the job provided is invalid: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go struct field Job.Count of type int` is too long, repetitive and the main message is at the very end, which is clear enough. We have a low hanging fruit to improve this be revisiting how we wrap these errors.

Though using schema validation before unmarshalling might add more value if it gives better error messages to the user. I am still in favour of keeping our validations in go for now, and maybe use schema validations to validate the structure before unmarshalling 

Fix patch:
diff --git a/cmd/cli/job/run.go b/cmd/cli/job/run.go
index aefc8ee1c7..98b1037b15 100644
--- a/cmd/cli/job/run.go
+++ b/cmd/cli/job/run.go
@@ -12,7 +12,6 @@ import (
 	"github.com/bacalhau-project/bacalhau/cmd/util/output"
 	"github.com/bacalhau-project/bacalhau/pkg/lib/marshaller"
 	"github.com/bacalhau-project/bacalhau/pkg/lib/template"
-	"github.com/bacalhau-project/bacalhau/pkg/models"
 	"github.com/bacalhau-project/bacalhau/pkg/publicapi/apimodels"
 	"github.com/bacalhau-project/bacalhau/pkg/publicapi/client/v2"
 
@@ -135,10 +134,7 @@ func (o *RunOptions) run(cmd *cobra.Command, args []string, api client.API) erro
 		}
 	}
 
-	// Turns out the yaml parser supports both yaml & json (because json is a subset of yaml)
-	// so we can just use that
-	var j *models.Job
-	err = marshaller.YAMLUnmarshalWithMax(byteResult, &j)
+	j, err := marshaller.UnmarshalJob(byteResult)
 	if err != nil {
 		return fmt.Errorf("%s: %w", userstrings.JobSpecBad, err)
 	}
diff --git a/pkg/lib/marshaller/utils.go b/pkg/lib/marshaller/utils.go
index 242fdbb6fc..a90839d1aa 100644
--- a/pkg/lib/marshaller/utils.go
+++ b/pkg/lib/marshaller/utils.go
@@ -2,13 +2,15 @@ package marshaller
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
 	"strings"
 
-	"github.com/bacalhau-project/bacalhau/pkg/models"
 	"github.com/c2h5oh/datasize"
 	"sigs.k8s.io/yaml"
+
+	"github.com/bacalhau-project/bacalhau/pkg/models"
 )
 
 type KeyString string
@@ -97,3 +99,90 @@ func normalizeIfApplicable(obj interface{}) {
 		normalizable.Normalize()
 	}
 }
+
+// UnmarshalJob unmarshalled `in` into a models.Job. It returns an error if:
+// - `in` cannot be marshaled to json.
+// - `in` contains an un-settable or unknown field.
+// - `in` cannot be marshaled into a models.Job.
+func UnmarshalJob(in []byte) (*models.Job, error) {
+	// json is a subset of yaml, if `in` is already json this is a noop.
+	jsonBytes, err := yaml.YAMLToJSON(in)
+	if err != nil {
+		return nil, fmt.Errorf("converting yaml to json: %w", err)
+	}
+
+	// unmarshal the job into a generic map
+	var raw map[string]interface{}
+	if err := json.Unmarshal(jsonBytes, &raw); err != nil {
+		return nil, err
+	}
+	// error(s) if un-settable or unknown fields are provided.
+	if err := validateRawJob(raw); err != nil {
+		return nil, err
+	}
+
+	var out *models.Job
+	if err := json.Unmarshal(jsonBytes, &out); err != nil {
+		switch v := err.(type) {
+		// json.UnmarshalTypeError describes a JSON value that as not appropriate for the value of the specified Go type.
+		// e.g. if the Count field was not an int, or the Name field was not a string
+		case *json.UnmarshalTypeError:
+			return nil, fmt.Errorf("field: '%s' in '%s' is invalid type: '%s' expected: '%s'", v.Field, v.Struct, v.Value, v.Type.String())
+		default:
+			return nil, err
+		}
+	}
+	return out, nil
+}
+
+// validateRawJob returns an error if `jobSpec` contains a job field that may not be set by the user or if unknown
+// fields were included in `jobSpec`.
+func validateRawJob(jobSpec map[string]interface{}) error {
+	var mErr error
+	// first check if any un-settable fields were provided.
+	for key := range jobSpec {
+		specKey := strings.ToLower(key)
+		if _, found := disallowedKeys[specKey]; found {
+			mErr = errors.Join(mErr, fmt.Errorf("field: '%s' in 'Job' is not allowed", key))
+		}
+	}
+	// remove all known fields from the provided spec.
+	for key := range jobSpec {
+		specKey := strings.ToLower(key)
+		if _, ok := disallowedKeys[specKey]; ok {
+			delete(jobSpec, key)
+		}
+		if _, ok := allowedKeys[specKey]; ok {
+			delete(jobSpec, key)
+		}
+	}
+	// any fields that remain are considered unknown
+	for key := range jobSpec {
+		mErr = errors.Join(mErr, fmt.Errorf("unknown field: '%s' in 'Job' is ignored", key))
+	}
+	return mErr
+}
+
+var (
+	// disallowedKeys contains system fields that users may not set when providing a job spec.
+	disallowedKeys = map[string]struct{}{
+		"id":         {},
+		"state":      {},
+		"version":    {},
+		"revision":   {},
+		"createtime": {},
+		"modifytime": {},
+	}
+	// allowedKeys contains fields users are permitted to set when providing a job spec.
+	allowedKeys = map[string]struct{}{
+		"name":        {},
+		"namespace":   {},
+		"type":        {},
+		"priority":    {},
+		"count":       {},
+		"constraints": {},
+		"meta":        {},
+		"labels":      {},
+		"tasks":       {},
+	}
+)


Test patch:
diff --git a/pkg/lib/marshaller/utils_test.go b/pkg/lib/marshaller/utils_test.go
new file mode 100644
index 0000000000..6ed7f27569
--- /dev/null
+++ b/pkg/lib/marshaller/utils_test.go
@@ -0,0 +1,138 @@
+//go:build unit || !integration
+
+package marshaller
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestUnmarshalJob(t *testing.T) {
+	t.Run("happy path", func(t *testing.T) {
+		var jobWithUnknownFields = `
+{
+  "Name": "A Simple Docker Job",
+  "Type": "batch",
+  "Count": 1,
+  "Tasks": [
+    {
+      "Name": "My main task",
+      "Engine": {
+        "Type": "docker",
+        "Params": {
+          "Image": "ubuntu:latest",
+          "Entrypoint": [
+            "/bin/bash"
+          ],
+          "Parameters": [
+            "-c",
+            "echo Hello Bacalhau!"
+          ]
+        }
+      }
+    }
+  ]
+}
+`
+		j, err := UnmarshalJob([]byte(jobWithUnknownFields))
+		assert.NotNil(t, j)
+		assert.NoError(t, err)
+	})
+	t.Run("job with unknown field", func(t *testing.T) {
+		var jobWithUnknownFields = `
+{
+  "Name": "A Simple Docker Job",
+  "Type": "batch",
+  "Selector": "nope",
+  "Count": 1,
+  "Tasks": [
+    {
+      "Name": "My main task",
+      "Engine": {
+        "Type": "docker",
+        "Params": {
+          "Image": "ubuntu:latest",
+          "Entrypoint": [
+            "/bin/bash"
+          ],
+          "Parameters": [
+            "-c",
+            "echo Hello Bacalhau!"
+          ]
+        }
+      }
+    }
+  ]
+}
+`
+		j, err := UnmarshalJob([]byte(jobWithUnknownFields))
+		assert.Nil(t, j)
+		assert.Error(t, err)
+		assert.ErrorContains(t, err, "unknown field: 'Selector' in 'Job' is ignored")
+	})
+	t.Run("job with un-settable field", func(t *testing.T) {
+		var jobWithUnknownFields = `
+{
+  "Name": "A Simple Docker Job",
+  "Type": "batch",
+  "Version": 1,
+  "Count": 1,
+  "Tasks": [
+    {
+      "Name": "My main task",
+      "Engine": {
+        "Type": "docker",
+        "Params": {
+          "Image": "ubuntu:latest",
+          "Entrypoint": [
+            "/bin/bash"
+          ],
+          "Parameters": [
+            "-c",
+            "echo Hello Bacalhau!"
+          ]
+        }
+      }
+    }
+  ]
+}
+`
+		j, err := UnmarshalJob([]byte(jobWithUnknownFields))
+		assert.Nil(t, j)
+		assert.Error(t, err)
+		assert.ErrorContains(t, err, "field: 'Version' in 'Job' is not allowed")
+	})
+	t.Run("job with invalid field type field", func(t *testing.T) {
+		var jobWithUnknownFields = `
+{
+  "Name": "A Simple Docker Job",
+  "Type": "batch",
+  "Count": "1",
+  "Tasks": [
+    {
+      "Name": "My main task",
+      "Engine": {
+        "Type": "docker",
+        "Params": {
+          "Image": "ubuntu:latest",
+          "Entrypoint": [
+            "/bin/bash"
+          ],
+          "Parameters": [
+            "-c",
+            "echo Hello Bacalhau!"
+          ]
+        }
+      }
+    }
+  ]
+}
+`
+		j, err := UnmarshalJob([]byte(jobWithUnknownFields))
+		assert.Nil(t, j)
+		assert.Error(t, err)
+		assert.ErrorContains(t, err, "field: 'Count' in 'Job' is invalid type: 'string' expected: 'int'")
+	})
+
+}