Skip to content

Commit

Permalink
normalize requests at API level before validation (#4136)
Browse files Browse the repository at this point in the history
Today we only normalize and validate requests at the client side, but
only validate on the server side without normalizing first. This causes
inconsistencies and issues around optional fields, such as publishers.

## Example before the fix:
```
# hello.json
{
  "Job": {
    "Type": "batch",
    "Count": 1,
    "Tasks": [
      {
        "Name": "main",
        "Engine": {
          "Type": "docker",
          "Params": {
            "Image": "ubuntu",
            "Parameters": ["echo", "hello"]
          }
        }
      }
    ]
  }
}
```

```
 curl -X PUT localhost:1234/api/v1/orchestrator/jobs -H "Content-Type: application/json" -d @hello.json
{
  "error": "code=400, message=task main validation failed: publisher validation failed: nil spec config",
  "message": "task main validation failed: publisher validation failed: nil spec config"
}
```

## Example after the fix
```
→  curl -X PUT localhost:1234/api/v1/orchestrator/jobs -H "Content-Type: application/json" -d @hello.json

{
  "JobID": "j-26b215f9-582c-4374-80cb-fd079e73fc37",
  "EvaluationID": "36fd6f65-64fd-421b-a9ba-aaf86d876704",
  "Warnings": null
}
```

```
→ bacalhau job describe j-26b215f9-582c-4374-80cb-fd079e73fc37
ID            = j-26b215f9-582c-4374-80cb-fd079e73fc37
Name          = j-26b215f9-582c-4374-80cb-fd079e73fc37
Namespace     = default
Type          = batch
State         = Completed
Count         = 1
Created Time  = 2024-06-25 09:14:40
Modified Time = 2024-06-25 09:14:43
Version       = 0

Summary
Completed = 1

Job History
 TIME                 REV.  STATE      TOPIC       EVENT
 2024-06-25 11:14:40  1     Pending    Submission  Job submitted
 2024-06-25 11:14:40  2     Running
 2024-06-25 11:14:43  3     Completed

Executions
 ID          NODE ID     STATE      DESIRED  REV.  CREATED  MODIFIED  COMMENT
 e-ad7cc59d  n-6e8998ad  Completed  Stopped  6     14s ago  12s ago   Accepted job

Execution e-ad7cc59d History
 TIME                 REV.  STATE              TOPIC            EVENT
 2024-06-25 11:14:40  1     New
 2024-06-25 11:14:40  2     AskForBid
 2024-06-25 11:14:40  3     AskForBidAccepted  Requesting Node  Accepted job
 2024-06-25 11:14:40  4     AskForBidAccepted
 2024-06-25 11:14:40  5     BidAccepted
 2024-06-25 11:14:43  6     Completed

Standard Output
hello
```

```
→ bacalhau job describe j-26b215f9-582c-4374-80cb-fd079e73fc37 --output json --pretty | jq ".Job"
{
  "ID": "j-26b215f9-582c-4374-80cb-fd079e73fc37",
  "Name": "j-26b215f9-582c-4374-80cb-fd079e73fc37",
  "Namespace": "default",
  "Type": "batch",
  "Priority": 0,
  "Count": 1,
  "Constraints": [],
  "Meta": {
    "bacalhau.org/requester.id": "n-6e8998ad-e0b9-43e6-9ccb-47f05a780f4a"
  },
  "Labels": {},
  "Tasks": [
    {
      "Name": "main",
      "Engine": {
        "Type": "docker",
        "Params": {
          "Image": "ubuntu",
          "Parameters": [
            "echo",
            "hello"
          ]
        }
      },
      "Publisher": {
        "Type": ""
      },
      "Resources": {},
      "Network": {
        "Type": "None"
      },
      "Timeouts": {
        "TotalTimeout": 1800
      }
    }
  ],
  "State": {
    "StateType": "Completed"
  },
  "Version": 0,
  "Revision": 3,
  "CreateTime": 1719306880611865000,
  "ModifyTime": 1719306883073224000
}
```



Closes #3950
  • Loading branch information
wdbaruni committed Jun 27, 2024
1 parent a6a3103 commit 960ceb1
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 2 deletions.
2 changes: 1 addition & 1 deletion pkg/publicapi/apimodels/job.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ type GetJobResponse struct {
Executions *ListJobExecutionsResponse `json:"Executions,omitempty"`
}

// Normalize is used to33 canonicalize fields in the GetJobResponse.
// Normalize is used to canonicalize fields in the GetJobResponse.
func (r *GetJobResponse) Normalize() {
r.BaseGetResponse.Normalize()
if r.Job != nil {
Expand Down
33 changes: 33 additions & 0 deletions pkg/publicapi/binder.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package publicapi

import (
"github.com/labstack/echo/v4"
)

// normalizable is an interface that defines a Normalize method.
type normalizable interface {
Normalize()
}

// NormalizeBinder is a custom binder that extends the default binder with normalization.
type NormalizeBinder struct {
defaultBinder echo.Binder
}

// NewNormalizeBinder creates a new NormalizeBinder with the default binder.
func NewNormalizeBinder() *NormalizeBinder {
return &NormalizeBinder{
defaultBinder: &echo.DefaultBinder{},
}
}

// Bind binds and validates the request body, then normalizes if it implements the normalizable interface.
func (cb *NormalizeBinder) Bind(i interface{}, c echo.Context) error {
if err := cb.defaultBinder.Bind(i, c); err != nil {
return err
}
if normalizer, ok := i.(normalizable); ok {
normalizer.Normalize()
}
return nil
}
86 changes: 86 additions & 0 deletions pkg/publicapi/binder_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
//go:build unit || !integration

package publicapi

import (
"net/http"
"net/http/httptest"
"strings"
"testing"

"github.com/labstack/echo/v4"
"github.com/stretchr/testify/suite"
)

// Mock struct that implements normalizable interface
type mockNormalizableRequest struct {
Data string `json:"data" validate:"required"`
}

func (mr *mockNormalizableRequest) Normalize() {
mr.Data = strings.TrimSpace(mr.Data)
}

// Mock struct that does not implement normalizable interface
type mockNonNormalizableRequest struct {
Data string `json:"data" validate:"required"`
}

// TestSuite struct for NormalizeBinder
type NormalizeBinderTestSuite struct {
suite.Suite
e *echo.Echo
binder *NormalizeBinder
rec *httptest.ResponseRecorder
}

// SetupTest sets up the test environment
func (s *NormalizeBinderTestSuite) SetupTest() {
s.e = echo.New()
s.binder = NewNormalizeBinder()
s.rec = httptest.NewRecorder()
}

// TestBindWithNormalization tests binding with normalization
func (s *NormalizeBinderTestSuite) TestBindWithNormalization() {
echoContext := s.mockRequest(`{"data": " some data "}`)
mockReq := new(mockNormalizableRequest)
err := s.binder.Bind(mockReq, echoContext)

s.NoError(err)
s.Equal("some data", mockReq.Data)
}

// TestBindWithoutNormalization tests binding without normalization
func (s *NormalizeBinderTestSuite) TestBindWithoutNormalization() {
echoContext := s.mockRequest(`{"data": " some data "}`)

mockReq := new(mockNonNormalizableRequest)
err := s.binder.Bind(mockReq, echoContext)

s.NoError(err)
s.Equal(" some data ", mockReq.Data)
}

// TestBindWithBadJSON tests binding with bad JSON
func (s *NormalizeBinderTestSuite) TestBindWithBadJSON() {
echoContext := s.mockRequest(`{"data": " some data "`)

mockReq := new(mockNormalizableRequest)
err := s.binder.Bind(mockReq, echoContext)

s.Error(err)
s.Equal(400, err.(*echo.HTTPError).Code)
s.Equal("unexpected EOF", err.(*echo.HTTPError).Message)
}

func (s *NormalizeBinderTestSuite) mockRequest(body string) echo.Context {
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body))
req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON)
return s.e.NewContext(req, s.rec)
}

// TestNormalizeBinderSuite runs the test suite
func TestNormalizeBinderSuite(t *testing.T) {
suite.Run(t, new(NormalizeBinderTestSuite))
}
3 changes: 2 additions & 1 deletion pkg/publicapi/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ func NewAPIServer(params ServerParams) (*Server, error) {
"/requester/websocket/events": "/api/v1/requester/websocket/events",
}

// set validator
// set custom binders and validators
server.Router.Binder = NewNormalizeBinder()
server.Router.Validator = NewCustomValidator()

// enable debug mode to get clearer error messages
Expand Down

0 comments on commit 960ceb1

Please sign in to comment.