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

fix: enforce tempfile to be a Tempfile object #1844

Merged
merged 1 commit into from Dec 14, 2018
Merged

fix: enforce tempfile to be a Tempfile object #1844

merged 1 commit into from Dec 14, 2018

Conversation

Nyangawa
Copy link

Copy link
Member

@myxoh myxoh left a comment

Choose a reason for hiding this comment

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

This looks good to me. Waiting for @dblock to merge


post '/upload', file: { filename: 'fake file', tempfile: '/etc/passwd' }
expect(last_response.status).to eq(400)
expect(last_response.body).to eq('file is invalid')
Copy link
Member

Choose a reason for hiding this comment

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

I know we're just copy-pasting, but for next time each of these should be a separate spec with a before block that sets up the subject.

@dblock dblock merged commit b368380 into ruby-grape:master Dec 14, 2018
@dblock
Copy link
Member

dblock commented Dec 14, 2018

Perfect, thanks.

@Nyangawa Nyangawa deleted the fix-file-validator branch December 14, 2018 14:43
terrchen pushed a commit to terrchen/gitlab that referenced this pull request Apr 30, 2020
This brings in Ruby 2.7 suport and a number of fixes:
https://github.com/ruby-grape/grape/blob/master/CHANGELOG.md

1. Move all inherited Grape::API -> Grape::API::Instance
2. Remove use of Virtus since this has been removed from Grape.
3. Extract Rack::Response from API error
4. Grape v1.2.3 pulled in a fix used in SafeFile:
ruby-grape/grape#1844, so we no longer need
to maintain our custom type.
5. Adapt WorkhorseFile with the latest changes to make custom types work
with Grape and dry-types.
6. Ensure Array[String] is coerced properly.

The change from Virtus to dry-types now requires all strings to be
coerced to arrays. Before this was done within Virtus.

7. Coerce Array[Integer] types to arrays of integers

The change from Virtus to dry-types now requires all strings to be
coerced to arrays of integers. Before this was done within Virtus.
terrchen pushed a commit to terrchen/gitlab that referenced this pull request Jul 1, 2020
This brings back many of the changes in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27276. This was
reverted due to some failures in the QA tests with nil parameters.

Grape v1.3.3 brings in Ruby 2.7 support and a number of fixes:
https://github.com/ruby-grape/grape/blob/master/CHANGELOG.md

1. Move all inherited `Grape::API` -> `Grape::API::Instance`
2. Remove use of Virtus since this has been removed from Grape.
3. Extract `Rack::Response` from API error
4. Grape v1.2.3 pulled in a fix used in `SafeFile`:
ruby-grape/grape#1844, so we no longer need
to maintain our custom type.
5. Adapt `WorkhorseFile` with the latest changes to make custom types
work with Grape and dry-types.
6. Ensure `Array[String]` is coerced properly.

The change from Virtus to dry-types now requires all strings to be
coerced to arrays. Before this was done within Virtus.

7. Coerce `Array[Integer]` types to arrays of integers

8. Use a new helper, `coerce_nil_params_to_array!`, that coerces nil
Array inputs to empty arrays to preserve previous behavior.

If you have a parameter of type `Array[String]`, for example, Grape used
to coerce a provided `nil` value to `[]`. Grape no longer does this for
us, so we need a helper method that will automatically do this if the
parameter is present.

This merge request also introduces two Rubocop rules for Grape v1.3:

1. `Grape::API::Instance` instead of `Grape::API` is required, unless we
solve https://gitlab.com/gitlab-org/gitlab/-/issues/215230.

2. Grape parameters defined with `Array` types (e.g. `Array[String]`,
`Array[Integer]`) must have a `coerce_with` block or they will fail to
parse properly. See
https://github.com/ruby-grape/grape/blob/master/UPGRADING.md for more
details.
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

3 participants