Skip to content

fix: define properly query parameters #660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 10, 2025
Merged

fix: define properly query parameters #660

merged 5 commits into from
Jul 10, 2025

Conversation

dbarrosop
Copy link
Member

@dbarrosop dbarrosop commented Jul 8, 2025

PR Type

Bug fix, Enhancement


Description

  • Fix handling of allowedRoles and metadata query parameters.

  • Update OpenAPI spec for correct query parameter definitions.

  • Parse metadata as JSON-encoded string in API handler.

  • Update tests to use JSON string for metadata.


Changes diagram

flowchart LR
  openapi["OpenAPI spec (openapi.yaml)"]
  server["Server handler (server.gen.go)"]
  types["API types (types.gen.go)"]
  tests["SignInProvider tests (sign_in_provider_test.go)"]

  openapi -- "Clarify allowedRoles/metadata param types" --> server
  openapi -- "Clarify allowedRoles/metadata param types" --> types
  server -- "Parse metadata as JSON string" --> types
  types -- "Update Metadata docstring" --> tests
  tests -- "Test metadata as JSON string" --> server
Loading

Changes walkthrough 📝

Relevant files
Enhancement
openapi.yaml
Update OpenAPI spec for allowedRoles and metadata query params

docs/openapi.yaml

  • Add style: form and explode: false for allowedRoles.
  • Change metadata to a JSON-encoded string in query parameters.
  • Update metadata description and schema to reflect JSON encoding.
  • +11/-7   
    Bug fix
    server.gen.go
    Fix and parse allowedRoles/metadata query parameters in handler

    go/api/server.gen.go

  • Fix allowedRoles to not require parameter and not explode.
  • Parse metadata query param as JSON string and unmarshal into map.
  • Improve error handling for invalid metadata JSON.
  • +177/-169
    Documentation
    types.gen.go
    Clarify Metadata field as JSON-encoded string                       

    go/api/types.gen.go

    • Update Metadata field comment to specify JSON-encoded string.
    +1/-1     
    Tests
    sign_in_provider_test.go
    Update test to use JSON string for metadata param               

    go/controller/sign_in_provider_test.go

    • Update test to provide metadata as JSON string.
    +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    github-actions bot commented Jul 8, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 704ed7b)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Query Parameter Parsing

    The new logic for parsing the metadata query parameter expects a JSON-encoded string and attempts to unmarshal it into a map. The reviewer should validate that this approach is compatible with all expected client requests and that error handling for malformed JSON is robust and user-friendly.

    if paramValue := c.Query("metadata"); paramValue != "" {
    
    	var value map[string]interface{}
    	err = json.Unmarshal([]byte(paramValue), &value)
    	if err != nil {
    		siw.ErrorHandler(c, fmt.Errorf("Error unmarshaling parameter 'metadata' as JSON: %w", err), http.StatusBadRequest)
    		return
    	}
    
    	params.Metadata = &value
    OpenAPI Spec Consistency

    The OpenAPI spec now defines metadata as a JSON object via application/json content in the query parameter. The reviewer should ensure this matches actual server-side parsing and is supported by API gateway/proxy layers, as some tools may not support JSON objects in query parameters.

    description: Additional metadata for the user (JSON encoded string)
    content:
      application/json:
        schema:
          type: object
          additionalProperties: true
          example:
            firstName: John
            lastName: Smith

    Copy link
    Contributor

    github-actions bot commented Jul 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Decode URL-encoded metadata before parsing

    The metadata query parameter is described as a URL-encoded JSON string, but the code
    attempts to unmarshal it directly without decoding. This will cause failures if the
    string is URL-encoded. Use url.QueryUnescape before unmarshalling to ensure correct
    parsing.

    go/controller/sign_in_provider.go [39-55]

     func getMetadaFromQueryParam(
     	rawMetadata *string,
     	logger *slog.Logger,
     ) (map[string]any, *APIError) {
     	if rawMetadata == nil {
     		return nil, nil
     	}
     
    +	decoded, err := url.QueryUnescape(*rawMetadata)
    +	if err != nil {
    +		logger.Error("error decoding metadata", logError(err))
    +		return nil, ErrInvalidRequest
    +	}
    +
     	var metadata map[string]any
    -	if err := json.Unmarshal([]byte(*rawMetadata), &metadata); err != nil {
    +	if err := json.Unmarshal([]byte(decoded), &metadata); err != nil {
     		logger.Error("error unmarshalling metadata", logError(err))
     		return nil, ErrInvalidRequest
     	}
     
     	return metadata, nil
     }
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies that the metadata parameter is URL-encoded and must be decoded before JSON unmarshalling, as per the OpenAPI spec and PR documentation. Failing to decode would break metadata parsing, so this is a critical fix for correct functionality.

    High

    @dbarrosop dbarrosop changed the title fix: query parameters must be strings fix: define properly query parameters Jul 8, 2025
    @dbarrosop dbarrosop closed this Jul 8, 2025
    @dbarrosop dbarrosop reopened this Jul 8, 2025
    Copy link
    Contributor

    github-actions bot commented Jul 8, 2025

    Persistent review updated to latest commit 704ed7b

    Copy link
    Contributor

    github-actions bot commented Jul 8, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @nunopato nunopato merged commit 736fc85 into main Jul 10, 2025
    8 checks passed
    @nunopato nunopato deleted the query-params-metadata branch July 10, 2025 14:11
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants