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

Avoid false positive from .try(:internal_resource) #980

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

bkiahstroud
Copy link
Contributor

Using #try on :internal_resource can yield unexpected results. If the method is undefined, it can return a truthy value instead of the typical nil.

E.g.

Hyrax::FileSet.try(:internal_resource) || 'hi'
=> #<Dry::Types::Result::Failure input=:internal_resource error=#<Dry::Struct::Error: [Hyrax::FileSet.new] can't convert Symbol into Hash>>

E.g.

Bulkrax.collection_model_internal_resource
=> #<Dry::Types::Result::Failure input=:internal_resource error=#<Dry::Struct::Error: [CollectionResource.new] can't convert Symbol into Hash>>

This was discovered while trying to export Valkyrie Resources from HykuUP (see screenshot)

Screenshot

image

@bkiahstroud bkiahstroud added the patch-ver for release notes label Sep 26, 2024
Using #try on :internal_resource can yield unexpected results.
If the method is undefined, it can return a truthy value instead of
the typical nil.

E.g.

```ruby
Hyrax::FileSet.try(:internal_resource) || 'hi'
=> #<Dry::Types::Result::Failure input=:internal_resource error=#<Dry::Struct::Error: [Hyrax::FileSet.new] can't convert Symbol into Hash>>
```
Copy link
Member

@orangewolf orangewolf left a comment

Choose a reason for hiding this comment

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

we can make this more compact I think and more efficient since raising and catching is expensive.

lib/bulkrax.rb Outdated Show resolved Hide resolved
@bkiahstroud bkiahstroud merged commit 92d6042 into main Sep 27, 2024
6 checks passed
@bkiahstroud bkiahstroud deleted the i-tried-so-hard-and-got-so-far branch September 27, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-ver for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants