-
Notifications
You must be signed in to change notification settings - Fork 8
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
Enhancement: Brand new DSL #109
Comments
Hi @synth , thank you for the suggestion! Agree this is a good addition and any PR would be very welcome. 👍 |
@synth Looks good! PR is very welcome. |
Cool. So, I forked this repo and ran An error occurred in a `before(:suite)` hook.
Failure/Error: DatabaseCleaner[:mongoid].strategy = :truncation, { only: "users" }
DatabaseCleaner::UnknownStrategySpecified:
The 'truncation' strategy does not exist for the mongoid ORM! Available strategies: deletion
# ./spec/support/db_cleaner.rb:15:in `block (2 levels) in <top (required)>'
# ------------------
# --- Caused by: ---
# NameError:
# uninitialized constant DatabaseCleaner::Mongoid::Truncation
# ./spec/support/db_cleaner.rb:15:in `block (2 levels) in <top (required)>' Any ideas? |
I remember that some time ago I was thinking about some kind of global masker configuration too. Actually, I liked the idea: everything is in one place, models are not polluted with User.unscoped.each do |usr|
# do something
usr.save!
end But if you want to do it with namespace :db do
task :setup_maskers => :environment do
Rails.application.eager_load!
class User
attr_masker :name
end
# or
User.class_exec do
attr_masker :name
end
end
end
# see: https://stackoverflow.com/a/47784838/304175
Rake::Task["db:mask"].enhance(["db:setup_maskers"]) At the moment I don't have a good Rails application to test that, so consider it a hint rather than a proven solution, sorry! Oh, and I'm not sure if it works well with tools like Spring or Zeus. I guess we should mention that in documentation. Also, if you just want re-using, then remember that anything what responds to MY_MASKER = lambda do |**args|
# do sth
end
# or
class MyMasker
def call(**args)
# do sth
end
end and use it: class User
attr_masker :name, masker: MY_MASKER
# or
attr_masker :name, masker: MyMasker.new
end |
Indeed, doing a Our initializer/Class.eval approach is working really well for us and in fact, we'll wrap the whole initializer in a Imagine something like this: global_condition = ->(model: ,**) { model.company_id.in?(valid_company_ids) }
email_masker = ->(**) { Faker::Name.email }
attr_masker User do
condition global_condition
mask :email, masker: email_masker
nullify :ip_address, :crypted_password
end Some notes about the above potential syntax:
Indeed, I concede that all of the above is possible without the DSL as you have described @skalee. In fact, we've met our somewhat advanced case as is by using the initializer/Class.eval approach and so we're super happy as is. But as rubyists, we strive for expressiveness, no? In any case, I'm blocked from proceeding to develop a PR due to the aforementioned issue just trying to run the tests. Happy to move forward with a potential global config DSL, if I can get unblocked. Otherwise, happy to leave as is and drop this idea as well. Cheers! :) |
This is more a legacy thing rather than a deliberate decision – this gem is forked from attr_encrypted. I really like the idea of moving configuration out of models to one place. Also, I am already convinced that the Rakefile trick is not obvious enough. IMO Furthermore, I agree that API should be updated too, as dedicated configuration file opens new possibilities. However, I need to think the details through, sorry. One reason is that we want to support more ORMs eventually, Sequel for instance. It was difficult to do that with current API and I want the new API to be elastic enough. Regarding DSL and block syntax, I hope that a good set of keyword arguments will be a better option, but I'll think about that too. For sure the new API should not involve class re-opening. So, I'll think on that and post updates here. |
Sounds good. So, to provide some more information about our specific cases that we discovered implementing this gem, here you go:
So, here's a contrived example that illustrates all of the above: attr_masker Company do # This class runs first
global_condition ->{ where.not(id: 1) } # sets global query scope
mask :domain, -> { Faker::Internet.domain_name }
end
attr_masker User do # This class runs second
global_condition ->{ where.not(company_id: 1) } # sets global query scope
nullify :crypted_password, :last_login_ip # runs first clearing out all the crud we don't need
mask :first_name, -> { Faker::Name.first_name } # runs second
mask :last_name, -> { Faker::Name.last_name } # runs third
mask :email, ->(model: ,**) { "#{model.first_name}.#{model.last_name}@#{model.company.domain}") # User runs after Company so we can guarantee the domain has been masked. Same goes for first/last name.
update_all "global_unsubscribe = true" # finally, do a final update_all on any other mass assignments like making sure everyone is globally unsubscribed to make sure they dont accidentally get emails (even if we've masked them).
end I know I'm being cheeky by getting rid of the keyword argument Anyway, let me know what you think when you get to it! This is just how we're using it and I think could definitely add value to others. Cheers! |
Thanks @synth for the good points, especially 5) since this is something we'd like to have as well. We hope to be able support most if not all of these use cases in the next API update. |
Thank you @synth for the great insights — the proposed DSL is thoughtful and demonstrates good use. Agree with @ribose-jeffreylau . |
Feature request for global configuration file (without API changes) has been extracted to #118. I prefer to keep this discussion open, so I've renamed the issue. |
For reference, this error is now fixed. |
First, Thank you for this gem!
So, I definitely see an argument for putting the
attr_masker
calls inside the models as its currently implemented. But there is also an argument for a centralized global configuration so all the masker calls are together and managed independently of the ORM (ActiveRecord) models. Further, this enables common conditions and custom maskers to be reused across models.Thanks to the beauty of ruby, this is easy to do. Here is a sample from our app
The above works beautifully but obviously is a bit cumbersome syntax. We can do better if this gem supported a higher level dsl. Here is one suggestion:
I think something like this would be fairly simple to implement. I could potentially submit a PR if you think this is useful. Let me know! Thanks again!
The text was updated successfully, but these errors were encountered: