DEV Community: Tomer Brisker

Decompose Conditionals and its tradeoffs

Tomer Brisker — Sun, 11 Dec 2022 12:51:03 +0000

Decomposing conditionals is a common refactoring strategy.
The main idea is that a complex if... else... statement is difficult to follow and understand, and can be decomposed into smaller pieces. But it does not come without a cost.

Instead of having multi-line conditions, the decompose conditional pattern states that conditional logic should be extracted to 3 separate methods, one for each part of the statement: one for the condition itself, one for the "true" branch and one for the "false" branch.

For example, if your code looks like this:



....
if baby.IsAwake() && baby.State == "Crying" && baby.TimeSinceLastMeal > 3 * time.Hour {
    if baby.Diaper.IsDirty() {
        baby.ChangeDiaper()
    } 
    bottle := NewBabyBottle()
    bottle.FillWater()
    bottle.AddMilkFormula()
    bottle.Shake()
    baby.Feed(bottle)
    baby.Burp()
} else {
    baby.Cuddle()
    baby.Play()
}
....

Decomposing the conditional would mean refactoring your code to look something like this:



....
if baby.IsHungry() {
    baby.ChangeAndFeed()
} else {
    baby.FunTime()
}
....
func (baby *Baby) IsHungry() {
    return baby.IsAwake() && baby.State == "Crying" && baby.TimeSinceLastMeal > 3 * time.Hour
}

func (baby *Baby) ChangeAndFeed() {
    if baby.Diaper.IsDirty() {
        baby.ChangeDiaper()
    } 
    bottle := NewBabyBottle()
    bottle.FillWater()
    bottle.AddMilkFormula()
    bottle.Shake()
    baby.Feed(bottle)
    baby.Burp()
}

func (baby *Baby) FunTime() {
    baby.Cuddle()
    baby.Play()
}

This change makes the main method flow much simpler to understand and follow, by abstracting away the complex logic that might not always be relevant for the reader.

Extracting the logic to separate methods also allows us to more easily reuse or refactor our code in the future, as the logic is better encapsulated. For example, once our baby starts eating solid foods, we can modify the ChangeAndFeed method to reflect that easily, without needing to touch any code that is not directly related to this change, and the same is true if the logic for checking if the baby is hungry ever changes - for example, once our baby starts eating at 4 hour intervals.

Another benefit from decomposing the conditional is that we can now write unit tests for the decomposed methods, ensuring that our logic is correct, and that it will not break due to future changes.

If decomposing conditionals is so great, than why don't we always do it?
Like everything in software engineering, there is a tradeoff.

When extracting logic to a separate method, there is additional overhead. While the overhead of a function call in most languages is minimal (except in very extreme cases, such as deep recursion or tightly resource-constrained environments), the overhead for the code reader is not. Software engineers spend much more time reading code than they do writing code, and we should always think about how we can make our code more readable. Jumping back and forth between method definitions and calls adds complexity for the reader who is interested in understanding what the code does, for example when reviewing or debugging it.

Take the following code for example. Which version is easier to understand and follow, this one:



if len(orgIDs) > maxItems || len(userIDs) > maxItems {
   return errors.BadRequest("Too Many Items")
}

or this one?



if validateIDsSize(orgIDs, userIDs) {
    return tooManyItems()
}
....
func validateIDsSize(ids ...[]string) bool {
    for _, idGroup := range ids {
        if len(idGroup) > maxItems {
            return true
        }
    }
    return false
}

func tooManyItems() error {
    return errors.BadRequest("Too Many Items")
}

The keyword here is Complexity. The question we should be asking is: "Is this piece of code complex enough that extracting it to a separate method will make it more readable or not?"
If the answer is that the code is clear enough to understand inline, we should avoid decomposing it to a separate function unless there is a very good reason to do so - such as a need for re-usability or testing.

The second example also shows another common risk we face when extracting logic to separate methods: a premature attempt at generalization. While the first version only checks two specific values, the second one tries to create a generic function that can accept any number of values.

The generic version of the check creates additional complexity to handle all possible cases, instead of a simpler check to handle the specific case we are facing.
If we have many similar cases with different values being checked, it could make sense to create a generic function that handles all cases.
However, we often find a generic function being only used in one or two places, in which case - the added complexity usually doesn't pay off when compared to a non-generic version.

We also don't yet know that all potential future cases will behave the same. For example, what if in the future different values will have different limits? The generic implementation assumes all values are limited by maxItems - meaning if this assumption ever changes, this method will become even more complex. A non-generic implementation is easier to change as we discover new requirements.

When used correctly, decomposing conditionals is a useful refactoring method that can make our code easier to understand, easier to test, and more reusable. However, before we approach refactoring our code, we must always keep in mind our end goals and consider the possible tradeoffs in each approach.

Attending recruiting fairs as an engineer

Tomer Brisker — Sun, 19 Jun 2022 18:49:20 +0000

Over the years, I’ve attended many student recruitment fairs representing Red Hat, including a couple in the recent weeks. I always try to go to at least one fair every year if possible.

But you might be wondering, is this the best usage of an engineer’s expensive work hours? Isn’t that the job of the talent acquisition team?
In my opinion, there is a lot of value for engineers to take part in these events, both for the organization and for the engineers themselves.

From the organization's perspective, let’s look at the several different goals from attending these events:

The obvious goal, recruiting new employees. While recruiting is the TA team’s speciality, engineers can provide additional value - for example, answering candidates’ technological questions better, or quickly identifying promising candidates that might get missed otherwise. Candidates often prefer to talk with actual engineers and trust them more than the people whose job is to promote the company to them but lack in technical skills.
Employee branding: A few lucky students might get hired - but the impact of the fair can be much wider. Having enthusiastic engineers take part in the fair can do a lot in terms of exciting students about the company and how cool it would be to work there. Some students might not yet fit the requirements of your open positions, and an engineer can give them pointers and advice on what they need to do to get accepted in the future. The students are usually at the start of their career, and if you manage to make a good impression on them they might consider your company a good option for their entire career. Building the company’s reputation as a good employer helps get more candidates into the hiring pipeline.
Potential future customers: If your company’s products or services are targeted at a technically-proficient user base, you might meet people who will go on to work at one of your future customers. Building awareness of your products could help with sales down the line, and who better to excite future engineers about how great your products are than the people working on them?
Connecting employees to company mission: When people promote the company, they need to learn more about what it does and how to explain its mission quickly to others. Representing the company is a great way to increase employee engagement with your company, and increase their awareness of the company strategy beyond their day-to-day work.

But why should you, as an engineer, take precious time away from your development work to go to a student fair?

Company-wide impact: One of the key criteria for career advancement in most companies is closely related to your impact. Affecting the hiring process is one of the most significant and long-lasting impacts you can make. The people hired today will be a key factor in the company’s future success, and they may even outlast you as an employee. If you manage to improve the quality of the candidates hired, this will have a compound impact for years to come.
Practice public speaking and interpersonal communication skills: If you’re an introvert like me, talking to strangers can be very tough. Recruitment fairs provide you with an opportunity to talk to people who you know are interested in what you have to say and hearing about your job. Sometimes you will get a somewhat pre-scripted pitch, so you don’t even have to prepare too much content - only describe the company to candidates and talk with them about your job.
Day out: Whether you normally work from home or the office, recruitment fairs are a great chance to break out from the regular routine, get a bit of vitamin D into your blood, stretch your muscles and breathe some fresh air. Your body will thank you for it.
Networking: Building a strong and wide network is key to growing your career and landing future opportunities. You will likely find yourself at the booth with people from your organization who you might not have worked with before. You might even make some connections with candidates or people staffing other companies’ booths. Take the time to get to know some of them better - who knows where they’ll be in the future and how they might be able to help you.

When an Array is not an Array

Tomer Brisker — Mon, 27 Aug 2018 10:32:44 +0000

The story of a simple array sort gone wrong

Part of the release process of the Foreman project includes extracting all strings from the source code for translation, and pulling updated translations from an online service we use called Transifex. As part of the release process for version 1.19.0, I ran the rake task which handles this step.

Unfortunately, it failed with a strange error message:

ArgumentError: comparison of Array with Array failed
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/gettext-3.2.9/lib/gettext/po.rb:270:in `sort_by’
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/gettext-3.2.9/lib/gettext/po.rb:270:in `sort_by_msgid’
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/gettext-3.2.9/lib/gettext/po.rb:227:in `sort’
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/gettext-3.2.9/lib/gettext/po.rb:208:in `to_s’
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/gettext-3.2.9/lib/gettext/tools/msgcat.rb:57:in `run’
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/gettext-3.2.9/lib/gettext/tools/msgcat.rb:32:in `run’
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/gettext-3.2.9/lib/gettext/tools/task.rb:391:in `block in define_po_file_task’
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/gettext_i18n_rails-1.8.0/lib/gettext_i18n_rails/tasks.rb:65:in `block (2 levels) in <top (required)>’
/home/tbrisker/.rvm/gems/ruby-2.5.1@foreman/gems/rake-12.3.1/exe/rake:27:in `<top (required)>’

Well, let’s look at the failing function’s code:

def sort_by_msgid(entries)
  entries.sort_by do |msgid_entry|
    # msgid_entry = [[msgctxt, msgid], POEntry]
    msgid_entry[0]
  end
end

Hmm… That’s quite odd, looks like a fairly simple sorting function. Looking at the gem’s git history indicates that this function hasn’t changed in ages — so why is it failing now? The comment seems pretty cryptic, and without digging too much into the gem’s internals it would be difficult to understand what it means. Time to pull out the ol’ debugger and stick a binding.pry breakpoint inside the function to try and understand what is going on here.

This is what one of the msgid_entry objects looks like:

[[nil, "Failed to fetch: "],
 #<GetText::POEntry:0x00000000144a3a80
  @extracted_comment="",
  @flags=[],
  @msgctxt=nil,
  @msgid="Failed to fetch: ",
  @msgid_plural=nil,
  @msgstr="Falha ao buscar:",
  @param_number=0,
  @param_type=[:msgid, :separator, :msgstr],
  @previous="",
  @references=["../app/assets/javascripts/application.js:74"],
  @translator_comment="",
  @type=:normal>]

It is an array that is composed of two elements — the first, an array that has two elements itself, and the second is a GetText::POEntry object, that is some sort of internal gettext object representing the translation. Now the comment in the code makes a bit more sense — the array contains a message context (in this case, nil) and a message ID, in this case, the string "Failed to fetch: ". The sorting function sorts the entries by the values of this array (msgid_entry[0]). So for some reason, one (or more) of the entries contains a problematic value in this array.

Stepping a few iterations in the loop didn’t cause the exception to be raised, and considering there are over 3,000 entries in the list (presumably, one for every string that needs translation), manually finding the problematic entry can be painstaking.

After a while of playing around with conditional breakpoints, the culprit was found:

[["(%s host)", "(%s hosts)"],
 #<GetText::POEntry:0x000000000f1ea780
  @extracted_comment="",
  @flags=[],
  @msgctxt="(%s host)",
  @msgid="(%s hosts)",
  @msgid_plural=nil,
  @msgstr=nil,
  @param_number=0,
  @param_type=[:msgctxt, :msgid, :msgstr],
  @previous="",
  @references=["../webpack/assets/javascripts/react_app/components/factCharts/index.js:86"],
  @translator_comment="",
  @type=:msgctxt>]

This was the only entry that had something other than nil for the first element of the first array — the message context. To make my work easier, the second element included the reference to the exact line of code where the string came from.

Looking at the code, the fix was clear — someone had a typo and used the incorrect method for marking the string for translation. So instead of using the method needed to mark the string as one that requires both singular and plural translation, the regular translation method was called, which apparently accepts an additional parameter for the context.

With the issue found and fixed, one question remained — why did this fail with the cryptic comparison of Array with Array failed error message?

To understand this, we need to dive a bit into how Ruby’s sort_by function works. Simply put, it accepts a block that gets as its argument the elements in the sorted object, and sorts them by the return value of the block. In our case, the entries are sorted by the value of the first element in each entry — the array containing the message context and ID.

But how does Ruby sort arrays? Turns out it sorts them by comparing one element at a time. To compare the elements, it calls the function <=>, which returns -1, 0, or 1 that indicate if the first element is smaller, equal or larger than the other. However, it can also return nil if the two elements can’t be compared, which is exactly what is happening here.

To illustrate, let’s look at what happens when we compare two strings:

[1] pry(main)> 'a' <=> 'foo'
=> -1

Since a is lexically smaller than foo, the function returns -1. But what happens when we try to compare a string with nil?

[2] pry(main)> nil <=> 'foo'
=> nil

As expected, since Ruby doesn’t know how to compare the two different object types, it returns nil. But what does that have to do with our error? Well, if you recall, Ruby compares arrays by iterating over the array elements. Now lets see what happens if it tries to compare arrays containing elements that can’t be compared:

[3] pry(main)> [[nil, 'foo'], ['bar', 'baz']].sort
ArgumentError: comparison of Array with Array failed
from (pry):4:in `sort'

One of the arrays being sorted by has a string for its first element, while the other has nil. This means Ruby doesn’t know how to sort these arrays, leading to an error. Unsurprisingly, since this is exactly the case as we were hitting with our translations, with one array containing a string where all others had nil, the same error was raised as when we tried the sorting in this example.

It works on my machine

Tomer Brisker — Sun, 08 Jul 2018 12:45:45 +0000

When Webpack is broken, but only in the packaged build — or how I created my first Webpack plugin

First, this story requires some background and a basic understanding of the architecture of the main project I’m working on, Foreman. Foreman is a Ruby on Rails application, used for infrastructure management. It has many plugins that add various functionality, which are written as Rails Engines. Since this application is used to manage critical infrastructure, it is installed on-premise and is shipped as packages — both .deb and .rpm. Many of the various plugins are also packaged and shipped as .rpm and .deb files.

The Foreman Project Logo

About 2 years ago we started modernizing our application’s front-end. To achieve this, it meant integrating Webpack and npm into our assets build process. This allowed us to use cool new technologies such as React and ES6 in our client side code, as well as start to solve long standing issues we had with our legacy JS code — such as lack of linting and unit-tests. The initial step of creating a Webpack build process took a while. One of the reasons is that RPM package builders must run in a disconnected environment, so we had to figure a way of providing all the node modules to the builder as RPMs at build time, but that is a story for an entire post on its own. Once that was solved, we started gradually migrating or replacing the legacy code-base with new, Webpack-managed code.

A major challenge we had recently was providing plugins with a way to leverage the existing npm and Webpack stack in Foreman core for writing their own client-side code. We didn’t want to force every plugin to create its own build pipeline, and we didn’t want to have multiple copies of every library included in each plugin’s JS bundle. This was solved with some clever engineering work by some of my colleagues. The solution involved compiling the Webpack assets for both plugins and the core application from the main application’s code base, while providing plugins with a vendor.js file that contains multiple shared libraries — such as React, Redux, Lodash and more. This worked perfectly fine during development, and even when compiling assets for production locally.

However, when we tried installing the nightly packages with plugins, something strange started happening. The core pages which included Webpack-managed assets worked fine, but the plugin pages requiring use of the plugin’s Webpack assets didn’t load. Looking in the console, it didn’t look promising:



TypeError: e[t] is undefined

e[t]

Now, part of the Webpack build process includes minifying the js code, making it smaller to download but much more difficult to debug. Luckily the browser developer tools knows how to prettify the code, so it is at least semi-readable. This pointed to the error being in the following function, on line 9:

Well, that’s not much better… but at least .exports hints at what this does. Comparing to the unminified and looking at the code around this function led me to understand that this is part of the Webpack code that handles module exports and makes them available to other modules, such as the plugin. But what is e[t]? And why is it failing here?

Since I know the failure occurs when e[t]===undefined, I set a conditional breakpoint on this line in my browser’s developer tools and reloaded the page. Now I could finally see what is happening here. e seems to be an object mapping hashes to functions:



» e
{0: ƒ, 00bb67fca9d5ae35c4aa: ƒ, 0139edb80f5e8e5773b7: ƒ, 01637e90596e3c444fa8: ƒ, 01d23de99989e6fe314f: ƒ, 01f59de879b1bcfbb8e7: ƒ, …}

And t is a hash — 2278734bfcaa57409dda in this case. But for some reason, this specific hash isn’t included in the e object. Digging inside the plugin’s minified code, I searched for the hash to try and figure out what it meant. This allowed me to make a guess at which module this hash should map to, but surprisingly, it was a module that should have been included inside vendor.js! So what happened here? Where did this hash come from? why was this working fine in the development environment? Looking in our Webpack config file, I found the following:

Git history indicated that HashedModuleIdsPlugin was introduced recently, in an attempt to allow plugins to use modules by creating a stable identifier for each module. Apparently, the default Webpack configuration generates a sequential ID for each module, making it difficult to use from inside plugins as the module IDs would change every time a module was added or removed. This explained why the hashes were only present in production, but not why this worked when compiling the Webpack assets for production locally and failed when using the bundles produced by our RPM builders. I needed to understand how this hash was generated.

Reading the Webpack documentation for this plugin, I found out that:

This plugin will cause hashes to be based on the relative path of the module , generating a four character string as the module id.

This led me to suspect that for some reason or other, when generating the Webpack bundles on the builders, the relative paths to the node modules were different for the plugins to those that were used when building Foreman core. However, from hashes this theory was very difficult to confirm. I needed some way to find the relative path that Webpack was using to generate the hash.

After some research, I found there is a plugin that does just that — NamedModulesPlugin. For some reason, the documentation (that has been deleted recently) claimed it should be used only in development and that it will show the relative path to the modules only when Hot Module Replacement is active. Looking at the code and trying it out, though, seemed to indicate that this is exactly what I needed to verify my hypothesis. When I replaced HashedModuleIdsPlugin with NamedModulesPlugin, the hashes were replaced with relative paths, both in the object mapping module identifiers to functions and in the code calling them. Locally, this continued working just as before with plugins, but the real test was when running with plugins built as RPMs.

Once the change to NamedModulesPlugin was merged into the project, it was time to rebuild the packages with the change and see if and what failed now. It took a couple of days to get the packages built again, since in the meantime some unrelated changes were merged that led to broken builds. But once they finally were, the moment of truth arrived — was my hypothesis correct?

“A bright neon on a brick wall in a store” by Austin Chan on Unsplash

Unsurprisingly, the same e[t] is undefined error showed up again. Only difference is that this time t should point to the actual module that was failing to load, so we can finally figure out why this was failing. Returning the conditional breakpoint to the vendor.js code allowed me to find what it was:



» t
"../../../../../../../usr/lib/node_modules/react/index.js"

React? That’s odd! We use React in multiple pages in the core application as well, and they work just fine, so it must be available in vendor.js! Quick CTRL+f for react in vendor.js led to identifying the culprit. This is the identifier that was present in vendor.js for the React module:



"../../../../usr/lib/node_modules/react/index.js"

Bingo! Notice the different number of leading ../’s compared to the identifier the plugin was looking for. This indicated that indeed, during the build process, Webpack was being run from different paths when building the plugin, compared to when building Foreman core.

Now that we finally got to the root of the issue, it was time to fix it. There were two different ways we could go about this:

Make sure that the build always occurs from the same root directory, leading to consistent module IDs.
Create a unique identifier for the modules that doesn’t care about where the build was started from, only about the module itself.

Option 1 could possibly work, but it required some deeper digging in our fairly complex build pipeline, followed by changes that may lead to other unexpected issues further down the process. It would also tie the whole build process to the Webpack configuration and reduce its flexibility in the future.

Option 2 required finding a way to make sure that module IDs are consistent across builds, regardless of what folder the build process was started from. One idea that was brought up was to use a hash of the module’s entire source code, but that would mean that every time any module is changed, all plugins would have to be rebuilt — since the hash would change as well. While for some changes it makes sense to also have plugins updated, usually module APIs don’t change much between versions, so that a minor change shouldn’t matter to the consuming plugins.

The solution that was finally decided on was to strip everything up to node_modules from the path for the module — so that React, for example, would get an ID of node_modules/react/index.js instead of something like ../../../../usr/lib/node_modules/react/index.js. We don’t really care where the node_modules folder is, and it shouldn’t matter to plugins either. This ID will remain consistent across builds, and for as long as the module doesn’t change its internal structure. But how can we do this?

To figure this out, I looked at the source code of the NamedModulesPlugin that is shipped with Webpack 3 (the version we are currently using). The code was surprisingly short — a loop over all modules, setting the ID using libIdent(). Some searching inside webpack code allowed me to understand the this function call will return the path to the module, without trying to dig to much into its internals.

Since the plugin doesn’t provide any means of modifying the ID, what was needed now was to make a new Webpack plugin that strips everything up to node_modules:

This code is almost the same as the original NamedModulesPlugin. The only change I added are lines 14–16, which strip out the extra part of the path from the module ID where needed.

As I assume others may run into this or similar issues in the future, I have extracted this plugin from our project to its own repository, and released it as a node module.

While this story was mostly written in the first person, as it depicts my perspective of the events, it is important to note that this solution, like everything I work on, is the result of a team effort.

Special thanks go to all those who helped in the process of fixing this issue, in no particular order: Daniel Lobato Garcia, Ewoud Kohl van Wijngaarden, Eric Helms, Avi Sharvit, Ohad Levy, Walden Raines, and anyone else who lent a hand and I forgot to mention.

This solution would also not have been possible if Webpack hadn’t been an open-source project — as I would not have been able to dig into its internals, extract and modify code from it otherwise.

Bug report to pull request in under 1 hour

Tomer Brisker — Mon, 12 Feb 2018 21:53:14 +0000

A user came in to our IRC channel today complaining about an issue with searching:

[16:31] <user> Why is it that when I search parent_hostgroup = “somerandom” in
the “Hosts” searchbox, where “somerandom” doesn’t match anything, it gives me a 
list of all hosts instead of none? I have some issues with this behavior when 
performing searches using the API
[16:40] <tbrisker> user: sounds like a bug, can you open an issue on 
projects.theforeman.org?
[16:49] <user> tbrisker, https://projects.theforeman.org/issues/22556

Luckily, this one was a quick fix. But let’s dive in together and understand why it failed in the first place.

We use a gem called Scoped Search to handle searches in our application. It allows us to provide our users with the ability to search the various resources in the application using a powerful query language and auto-completer. While for most cases scoped search Just Works™, sometimes more complex logic is needed for certain queries. This is possible by telling scoped search to use an external method to generate the query by passing an :ext_method parameter to the search definition.

The case of parent_hostgroup is one such case. A host in Foreman can belong to a host group, which in turn might be a child of one or more host groups. To allow searching by parent host group, i.e. any hosts that belong to a certain host group or any of its children, an external method is needed:

def search_by_hostgroup_and_descendants(key, operator, value)
  conditions = sanitize_sql_for_conditions(["hostgroups.title #{operator} ?", value_to_sql(operator, value)])
  # Only one hostgroup (first) is used to determined descendants. Future TODO - alert if result results more than one hostgroup
  hostgroup     = Hostgroup.unscoped.with_taxonomy_scope.where(conditions).first
  hostgroup_ids = hostgroup.subtree_ids
  if hostgroup_ids.any?
    opts = "hosts.hostgroup_id IN (#{hostgroup_ids.join(',')})"
  else
    opts = "hosts.id < 0"
  end
  {:conditions => opts}
end

Can you spot the bug?

It’s on line 5. If no host groups match the search on line 4, hostgroup will be nil. That means that hostgroup.subtree_ids should raise an exception:

NoMethodError: undefined method `subtree_ids' for nil:NilClass

That will be easy to fix, we just need to make sure that the hostgroup is present before collecting its sub-tree ids:

def search_by_hostgroup_and_descendants(key, operator, value)
  conditions = sanitize_sql_for_conditions(["hostgroups.title #{operator} ?", value_to_sql(operator, value)])
  # Only one hostgroup (first) is used to determined descendants. Future TODO - alert if result results more than one hostgroup
  hostgroup     = Hostgroup.unscoped.with_taxonomy_scope.where(conditions).first
  if hostgroup.present?
    hostgroup_ids = hostgroup.subtree_ids
    opts = "hosts.hostgroup_id IN (#{hostgroup_ids.join(',')})"
  else
    opts = "1 < 0"
  end
  {:conditions => opts}
end

And sure enough, less then an hour after the initial report, I had a pull request ready, which also included a unit test to ensure this function won’t break again in the future:

[17:23] <tbrisker> user: https://github.com/theforeman/foreman/pull/5249
[17:24] <user> tbrisker, thanks! :)

34 minutes of work and one happy user!

Since I try to follow the Boy Scout Rule, I also fixed a minor issue on line 9: the idea here is to return a condition that is always evaluated to false, thereby returning no results for the search. However, when the DB sees a condition like host.id < 0, it checks if any of the IDs are in fact smaller then 0. When you have tens of thousands of objects, this condition combined with other conditions (like default scope, default order, STI type etc.), even when executed against the index, can still take some time:

foreman=# explain analyze SELECT “hosts”.* FROM “hosts” WHERE “hosts”.”type” IN (‘Host::Managed’) AND ((hosts.id < 0)) ORDER BY “hosts”.”name” ASC;
 QUERY PLAN 
— — — — — — — — — — — — — — — — — — — — — — — — — — — — — — — — — 
 Sort (cost=62.19..62.20 rows=4 width=6134) (actual time=17.702..17.702 rows=0 loops=1)
 Sort Key: name
 Sort Method: quicksort Memory: 25kB
 -> Bitmap Heap Scan on hosts (cost=16.50..62.15 rows=4 width=6134) (actual time=17.692..17.692 rows=0 loops=1)
 Recheck Cond: ((type)::text = ‘Host::Managed’::text)
 Filter: (id < 0)
 Rows Removed by Filter: 43835
 Heap Blocks: exact=1429
 -> Bitmap Index Scan on index_hosts_on_type (cost=0.00..16.50 rows=12 width=0) (actual time=9.268..9.268 rows=43835 loops=1)
 Index Cond: ((type)::text = ‘Host::Managed’::text)
 Planning time: 0.359 ms
 Execution time: 17.808 ms
(12 rows)

Ok, that’s not too bad 😄

But on the other hand, when the RDBMS sees a condition like 1 < 0 it immediately evaluates it to false and returns much faster (by about 3 orders of magnitude):

foreman=# explain analyze SELECT "hosts".* FROM "hosts" WHERE "hosts"."type" IN ('Host::Managed') AND ((1 < 0)) ORDER BY "hosts"."name" ASC;
 QUERY PLAN 
------------------------------------------------------------------
 Sort (cost=0.01..0.02 rows=0 width=6134) (actual time=0.011..0.011 rows=0 loops=1)
 Sort Key: name
 Sort Method: quicksort Memory: 25kB
 -> Result (cost=0.00..0.00 rows=0 width=6134) (actual time=0.001..0.001 rows=0 loops=1)
 One-Time Filter: false
 Planning time: 0.241 ms
 Execution time: 0.077 ms
(7 rows)

Not a huge gain, but doesn’t hurt either. Perhaps learning this trick may come in handy for other cases, where it may lead to a more significant gain.

But wait, the user wasn’t actually complaining about an error. They complained that when the parent host group doesn’t exist, all hosts are returned instead of none of them. Why didn’t they get the NoMethodError? That’s peculiar!

Good thing the Scoped Search gem is Open Source, I can take a look inside and try to figure out what happened.

Here is the code that handles the external method definitions inside Scoped Search:

def to_ext_method_sql(key, operator, value, &block)
  raise ScopedSearch::QueryNotSupported, "'#{definition.klass}' doesn't respond to '#{ext_method}'" unless definition.klass.respond_to?(ext_method)
  conditions = definition.klass.send(ext_method.to_sym,key, operator, value) rescue {}
  raise ScopedSearch::QueryNotSupported, "external method '#{ext_method}' should return hash" unless conditions.kind_of?(Hash)
  sql = ''
  conditions.map do |notification, content|
    case notification
      when :include then yield(:include, content)
      when :joins then yield(:joins, content)
      when :conditions then sql = content
      when :parameter then content.map{|c| yield(:parameter, c)}
    end
  end
  return sql
end

Line 3 is the line that actually calls the external method when needed.

But what is that rescue {} doing there? That means that any error that occurs in the external method is swallowed silently, and the search will execute as if there was no condition at all! This is effectively suppressing any errors inside the external method, which is considered bad practice in Ruby and in programming in general. There are two reasons this is considered bad practice:

A user hitting an error (for example, by sending an invalid search query) will receive no indication that something went wrong. They may incorrectly assume the results they received were correct, which could lead them to take further actions that are mistaken. In our example, say a user wants to create a script that deletes all hosts in a certain host group and its children. If they mistype the host group name, instead of receiving an error or no results, they may end up deleting all of their hosts!
The developer creating the method who’s exceptions are being suppressed may think it is functioning correctly, and receive no indication that there is any issue in it. In this case, looking at the git history, this bug has existed for 3.5 years without being noticed!

So, thanks to the power of Open Source, I fixed the issue in Scoped Search and sent the maintainers a Pull Request as well:

def to_ext_method_sql(key, operator, value, &block)
  raise ScopedSearch::QueryNotSupported, "'#{definition.klass}' doesn't respond to '#{ext_method}'" unless definition.klass.respond_to?(ext_method)
  begin
    conditions = definition.klass.send(ext_method.to_sym, key, operator, value)
  rescue StandardError => e
    raise ScopedSearch::QueryNotSupported, "external method '#{ext_method}' failed with error: #{e}"
  end
  raise ScopedSearch::QueryNotSupported, "external method '#{ext_method}' should return hash" unless conditions.kind_of?(Hash)
  sql = ''
  conditions.map do |notification, content|
    case notification
      when :include then yield(:include, content)
      when :joins then yield(:joins, content)
      when :conditions then sql = content
      when :parameter then content.map{|c| yield(:parameter, c)}
    end
  end
  return sql
end

Now let’s just hope they like it enough to merge it into the project! 😃

Thank you for reading, I would be happy to hear feedback in the comments below or on twitter.

Strong Parameters in Rails — down the rabbit hole

Tomer Brisker — Wed, 17 Jan 2018 22:40:45 +0000

It all started with a bug report (doesn’t it always?). A user was complaining that when they passed an incorrect argument to our API by mistake, they didn’t get any error message. Sounds like a quick fix, right?

Wrong.

Some background first, simplified greatly for the purpose of this post. Our application, like many others, has users. A user can belong to multiple organizations. Our REST API allows updating a user by PUTting a JSON object to the relevant endpoint, for example:

{ "user": { "organization_ids": [1,2,3]}}

But our user had a typo. This is the JSON they sent:

{ "user": { "organization_ids": 1}}

It seemed to work fine — they got no error message. Perhaps it worked even too well: When they tried to pass a non-existing organization ID, it still returned a successful response! In fact, whenever they forgot to wrap the ID in an array, that parameter was simply ignored. No error response, no mention in the logs of something being wrong, nothing.

After I reproduced the bug in my development environment, I started digging in. With some help from pry inside the controller, I saw that while the organization_ids parameter was present in the request, it wasn’t there in the user_params object that contains the allowed parameters for assignment. This pointed me in the direction of Strong Parameters.

Strong Parameters is a feature of Rails that prevents assigning request parameters to objects unless they have been explicitly permitted. It has its own DSL (Domain Specific Language, or in other words, a predefined syntax it understands), that allows you to indicate what parameters should be allowed. It also lets you indicate if each parameter should be a hash, array or scalar (i.e. integer, string, etc.), as well as some other functionality that is not relevant for this post.

Here is an excerpt from our definition of permitted parameters in the users controller:

params.permit :name, :login, :mail, :organization_ids => [], ...

Ok, so Strong Parameters expects an array for the :organization_ids parameter, but it gets an integer instead. So why am I not seeing any indication that it failed in the logs?

Time to dig into the Rails code. Good thing it’s Open Source and I can just read it myself! Here is the function that does the actual filtering:

def permit(*filters)
  params = self.class.new

  filters.flatten.each do |filter|
    case filter
    when Symbol, String
      permitted_scalar_filter(params, filter)
    when Hash
      hash_filter(params, filter)
    end
  end

  unpermitted_parameters!(params) if self.class.action_on_unpermitted_parameters

  params.permit!
end

So looks like it does have some sort of handling for the unpermitted parameters… Lets take another step inside the code:

def unpermitted_parameters!(params)
  unpermitted_keys = unpermitted_keys(params)
  if unpermitted_keys.any?
    case self.class.action_on_unpermitted_parameters
    when :log
      name = "unpermitted_parameters.action_controller"
      ActiveSupport::Notifications.instrument(name, keys: unpermitted_keys)
    when :raise
      raise ActionController::UnpermittedParameters.new(unpermitted_keys)
    end
  end
end

But wait, what is this action_on_unpermitted_parameters thing? Turns out this is a configuration option you can define in your application initialization. So I looked in our application.rb and this is what we had defined:

config.action_controller.action_on_unpermitted_parameters = :strict

Well, this explains why we saw nothing! This isn’t actually a value this option expects!

Digging in git history, turns out the :strict option is actually a left-over from Rails 3 days — when it was passed to config.active_record.mass_assignment_sanitizer, which was a previous method of sanitizing input that accepted the :strict option. When we implemented Strong Parameters in preparation for the Rails 5 upgrade, this option was renamed correctly, but its value remained the same. Which means that for about a year and half, we were silently dropping all of the filtered parameters instead of raising an exception.

Great! Now all I have to do is replace :strict with :raise, write a little bit of error handling code to give the user a nice friendly message informing them which of the parameters they passed was wrong and why and I’m done! Shouldn’t take too long…

After a couple of hours of work, I realized that due to the way that Strong Parameters is currently implemented, there is no easy way of telling whether the parameter was filtered out because the name wasn’t permitted or because the value wasn’t the correct type (which was the original issue — passing an integer when the filter only accepted an array). OK, no worries, for now let’s just display a general message informing which parameter was the problematic one, that should be enough to help the user figure out what’s wrong.

So I added something like this in the base API controller:

rescue_from ActionController::UnpermittedParameters do |error|
  message = "Invalid parameter: %s. " % error.params.to_sentence
  message << 'Please verify that the parameter name is valid and the values are the correct type.'
  render_error 'param_error', :status => :bad_request, :locals => { :exception => error, :message => message }
end

Excellent! Good day’s work, let’s open a PR and see what the CI says!

About an hour later…

(we have an extensive test suite that is run against different Ruby versions and different databases)

Jenkins reports that tests have failed. 636 failed tests to be exact. Yikes! I guess a lot of code was changed in the year-and-half since we made the change to Strong Parameters.

Digging one more level down the Rails code, the unpermitted_keys that raise an exception are calculated this way:

def unpermitted_keys(params)
  keys - params.keys - always_permitted_parameters
end

Luckily, turns out always_permitted_parameters is actually another config option. Despite what its name suggests, it is actually a list of parameters that are filtered out but don’t raise an error when they are. By default, it is set to %w( controller action ), since these two parameters are always sent by Rails, but it can easily be overridden in the application config. This is useful for parameters that you want to pass but don’t need to be assigned to the models — for example, number of entries to display per page in index views. I added a few common parameters to this list, like so:

config.action_controller.always_permitted_parameters = %w(controller action format page per_page search locale utf8 _method authenticity_token locale _ie_support apiv id)

This got me down to 237 failing tests. Not bad for a quick, one line change.

However, now the failing tests will need individual handling. Some may be easily fixed by adding some more common parameters that I didn’t think about to the list, but some of them are actually broken and have been for a while — we just didn’t realize it because we were silently filtering out many parameters which may have caused the tests to fail. In the following clean-up, we may even find some new bugs that were missed because of this.

On the positive side, getting this fixed properly will give us several benefits:

Our users will receive informative feedback when they send an invalid parameter to the API. Even though at this point we still won’t be able to tell them what’s wrong, they will at least know which parameter(s) are causing issues, and won’t think an action succeeded when in fact some of the parameters were silently filtered out.
Developers will have an easier time when debugging. Instead of wasting hours trying to figure out why the parameter they expected wasn’t available in the controller, they will get a clear exception that they can handle.
Our tests will be better. Silently failing during tests leads us to an false feeling that everything is fine. Since we have a large test suite with good coverage (sadly not 100% yet), we often rely on tests when doing code review as an indication that new code isn’t introducing a regression.

I think any one of these is in itself worth my effort, not to mention getting all three at once.

My key takeaways from all of this:

Failing silently is a Bad Idea™. In this case, we missed the fact that after the change to Strong Parameters, the :strict option no longer produces the desired outcome — raising an error when filtering out parameters.
Open Source is awesome. Thanks to the fact that the Rails code is open, I was able to figure out the cause of this bug in just a few hours.
Don’t be afraid to dig in deep. Even if that means going into the code of some of your dependencies or their dependencies. I learned a whole lot in my career from digging deep - both in terms of understanding our stack better and of reading some great code written by very talented developers. If you find a bug in some dependency’s code — fix it and send a PR! That way everyone will benefit from the fix, and you won’t have to create an ugly workaround in your code.
Tests are important. Making sure they work the way you expect them to work is even more important. Don’t just check the request completed successfully, check that it did what it was supposed to do.
Code review is important. If this issue was caught in code review 1.5 years ago, we wouldn’t have to fix hundreds of tests now. Even though the PR did undergo review, it was a huge PR — 257 files changed, which makes sense when changing such a fundamental aspect that touches practically every part of the application. However, such a large amount of changes makes it easy to miss something when reviewing. Whenever possible, small PRs are always better, as the chance that something will be missed is much smaller.
Good documentation is important, especially if you are writing a library others depend on. Thanks to the Rails docs, I was quickly able to understand what each function does and identify which configuration options I need to set.

Thank you for reading this! I would be happy for any feedback in the comments, or by DM on Twitter.

P.S. Thanks Dafna Rosenblum for writing the post that led me to write this one, about 2 years after I told myself I should start a blog.