Skip to content

Conversation

@bogdan
Copy link
Contributor

@bogdan bogdan commented Dec 6, 2025

After update to grape 3.0, hidden attributes started to appear inside required params. I am not sure why hidden parameters were not required when the same version of grape-swagger-entity was used with earlier versions of grape-swagger. But I think it requires a fix anyway.

The bug was likely introduced here: b2a7b90

cc @numbata

Copy link
Collaborator

@numbata numbata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! I ran into this same issue yesterday and started working on a fix on the grape-swagger side as well. I think both fixes deserve to exist - yours handles it properly at the grape-swagger-entity side (where the root cause is), while the grape-swagger fix will act as a safeguard for similar issues that might come from other grape-swagger-* gems, not just grape-swagger-entity. Merging!

@numbata
Copy link
Collaborator

numbata commented Dec 6, 2025

@bogdan Can you update CHANGELOG.md ?

- Update `required_params` method to skip hidden attributes when determining required parameters.
- Modify `response_model_spec` to reflect changes in required attributes, removing `hidden_attr`.
- Add a test in `parser_spec` to ensure hidden attributes are not marked as required.
@numbata
Copy link
Collaborator

numbata commented Dec 6, 2025

@dblock Hi! I found that the GitHub token hardcoded in the Danger workflows (in grape-swagger-entity and other grape repos) has expired. The token is returning 401 Bad credentials when Danger tries to authenticate. Do you have a new token available? If so, instead of hardcoding it in the workflow, can we store a new one as a repository secret (DANGER_TOKEN) and referencing it via ${{ secrets.DANGER_TOKEN }} to keep it out of version control?

@dblock
Copy link
Member

dblock commented Dec 6, 2025

@numbata I opened ruby-grape/grape#2630 with some details, feel free to pick it up! In the meantime removing danger or ignoring it is probably the easiest way, but I'd prefer a real fix.

@bogdan
Copy link
Contributor Author

bogdan commented Dec 24, 2025

@numbata any change merging this without ruby-grape/grape#2630 being resolved?

@numbata
Copy link
Collaborator

numbata commented Dec 24, 2025

@bogdan I plan to fix the Danger issue in the repo today (it just needs a GitHub Actions workflow update), and then we can move forward with this PR.
Sorry for delay.

@numbata
Copy link
Collaborator

numbata commented Dec 25, 2025

@bogdan can you rebase from master branch, please?

@github-actions
Copy link

github-actions bot commented Dec 25, 2025

Danger Report

No issues found.

View run

@numbata
Copy link
Collaborator

numbata commented Dec 25, 2025

Let's fix a little, so I can merge it?

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 78d143c..67cc731 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -37,7 +37,7 @@ Metrics/AbcSize:
 # Offense count: 2
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 117
+  Max: 120
 
 # Offense count: 2
 # Configuration parameters: AllowedMethods, AllowedPatterns.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 66235e1..1044d41 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,7 +7,7 @@
 #### Fixes
 
 * Your contribution here.
-* [#87](https://github.com/ruby-grape/grape-swagger-entity/pull/87): remove hidden attributes from required [@bogdan](https://github.com/bogdan)
+* [#87](https://github.com/ruby-grape/grape-swagger-entity/pull/87): remove hidden attributes from required - [@bogdan](https://github.com/bogdan).
 * [#88](https://github.com/ruby-grape/grape-swagger-entity/pull/88): Update danger workflows - [@numbata](https://github.com/numbata).
 
 ### 0.7.0 (2025/08/02)

numbata and others added 2 commits December 25, 2025 13:53
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@dblock.org>
Increased maximum class length from 117 to 120.
@numbata numbata merged commit 7baad46 into ruby-grape:master Dec 25, 2025
11 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.

3 participants