Skip to content
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

Ensure inherited nested exposures are merged #72

Merged
merged 3 commits into from
May 29, 2024

Conversation

pirhoo
Copy link
Contributor

@pirhoo pirhoo commented May 19, 2024

Hello,

This PR want to fix a limitation of this gem with nested expose on child entities.

Problem

I noticed that when an entity inherits from another, nested exposures are not merged (as opposed to Grape Entity) in the definition object. For instance this won't work:

class Nested < Grape::Entity
  expose :nested, documentation: { type: Hash, desc: 'Nested entity' } do
    expose :some1, documentation: { type: 'String', desc: 'Nested some 1' }
    expose :some2, documentation: { type: 'String', desc: 'Nested some 2' }
  end
end

class NestedChild < Nested
  expose :nested, documentation: { type: Hash, desc: 'Nested entity' } do
    expose :some3, documentation: { type: 'String', desc: 'Nested some 3' }
  end
end

Would produce this definition:

{
  "definitions": {
    "Nested": {
      "type": "object",
      "properties": {
        "nested": {
          "type": "object",
          "properties": {
            "some1": {
              "type": "string",
              "description": "Nested some 1"
            },
            "some2": {
              "type": "string",
              "description": "Nested some 2"
            }
          },
          "description": "Nested entity"
        }
      }
    },
    "NestedChild": {
      "type": "object",
      "properties": {
        "nested": {
          "type": "object",
          "properties": {
            "some1": {
              "type": "string",
              "description": "Nested some 1"
            },
            "some2": {
              "type": "string",
              "description": "Nested some 2"
            }
          },
          "description": "Nested entity"
        }
      }
    }
  }
}

Solution

Instead of listing params from the first match of a given property (with find_by) we use all exposed properties on the current model (with select_by). This allows to get exposed properties both from Nested and NestedChild.

With those changes, the new property "some3" is now visible in the definition:

{
  "definitions": {
    "Nested": {
      "type": "object",
      "properties": {
        "nested": {
          "type": "object",
          "properties": {
            "some1": {
              "type": "string",
              "description": "Nested some 1"
            },
            "some2": {
              "type": "string",
              "description": "Nested some 2"
            }
          },
          "description": "Nested entity"
        }
      }
    },
    "NestedChild": {
      "type": "object",
      "properties": {
        "nested": {
          "type": "object",
          "properties": {
            "some1": {
              "type": "string",
              "description": "Nested some 1"
            },
            "some2": {
              "type": "string",
              "description": "Nested some 2"
            },
            "some3": {
              "type": "string",
              "description": "Nested some 3"
            }
          },
          "description": "Nested entity"
        }
      }
    }
  }
}

Thanks!

@pirhoo pirhoo changed the title Ensure inherited nested exposure are merged Ensure inherited nested exposures are merged May 19, 2024
@dblock
Copy link
Member

dblock commented May 29, 2024

Thanks! Add a line to CHANGELOG and we're good to go.

@pirhoo
Copy link
Contributor Author

pirhoo commented May 29, 2024

Done ;) Thanks!

@dblock
Copy link
Member

dblock commented May 29, 2024

@pirhoo you'll have to fix that changelog line :) sorry

@pirhoo
Copy link
Contributor Author

pirhoo commented May 29, 2024

Oups, sorry!

@dblock dblock merged commit 9f44aa2 into ruby-grape:master May 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants