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

Rails params whitelist marked #105

Open
ivdma opened this issue Mar 4, 2016 · 2 comments
Open

Rails params whitelist marked #105

ivdma opened this issue Mar 4, 2016 · 2 comments

Comments

@ivdma
Copy link

ivdma commented Mar 4, 2016

Two different Rails params whitelists are marked as duplicates with a threshold of 23.

# Controller A
params.require(:model_a).permit(:name, :description, :automatic, other_model_ids: []).tap do |model_a_params|
      model_a_params[:settings] = params[:model_a][:settings]
end

# Controller B
params[:model_b].permit(:happened_at, :other_model_id, :some_type, some_other_models_ids: [])
                         .tap { |whitelisted| whitelisted[:reason] = params[:model_b][:reason] }

There are some similarities, but this shouldn't be a duplication error.

@wfleming
Copy link
Contributor

Hi @ivdma,

Right now the duplication algorithm effectively compares syntax nodes based on both what kind of expression they represent, as well as the contents of those expressions. When the kinds of the expressions are the same, but the contents vary (which looks to be the case here), the engine considers that "similar" duplication to distinguish it from "identical" duplication.

The detection of "similar" duplication like your case is one that, while it can turn up helpful results in many cases, is unfortunately also prone to these kinds of false-positives that are easy to distinguish as a human but not easy to make a decision about within an algorithm. We are continuing to investigate ways of improving the algorithm for these kinds of cases, but need to be careful that in avoiding false-positives we don't swing too far and stop returning useful instances of similar code. So for the time being we don't consider this behavior a bug since this is the expected behavior of Flay, the underlying tool this engine uses for detecting duplication.

There are several workarounds available to you in the meantime:

  1. You can ignore particular issues using their fingerprints.

  2. You can tune the mass threshold of duplicate code that the engine will report. This will effect both similar & identical issues. (Ruby's default mass threshold is 18.)

    engines:
      duplication:
        enabled: true
        config:
          ruby:
            mass_threshold: 25
  3. If you would prefer not to see reports of similar code at all, you can disable that check entirely. If you do this, the engine will only report instances of code it sees as identical.

    engines:
      duplication:
        enabled: true
        checks:
          Similar code:
            enabled: false

@zenspider
Copy link
Contributor

See #190 for my proposed solution to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants