DEV Community: Daniel Ferraz

When belongs_to association in Rails goes weird

Daniel Ferraz — Sun, 17 Nov 2019 22:49:04 +0000

ActiveRecord is one of those many things in Rails that can be both good and bad depending on the perspective, the size of the project, how it's used, etc.

It's loved when it performs a really good job on abstracting away the specifics of the DB being used (like any good ORM should do), when it makes really easy to define and bootstrap a fully functional app with the domain classes and the persistence mechanism in place, when it helps keeping the data valid and consistent through validators, when it facilitates the migration and rollback of DB schema changes, etc.

It's hated when it makes really easy to tighly couple the application domain and logic to the persistence layer, when it faciliates the proliferation of God objects since it's so easy to put more and more behavior inside the record models and pass them around everywhere, when your domain classes become a big ball of mud knowing too much about everything and with hooks everyhwere, when it makes your tests slow, etc.

But one thing that it should strive, apart from the pros and cons, is to maintain consistency on its behavior and usage. Recently I noticed a weird behavior with the belongs_to association which is somewhat inconsistent with the other associations and hurts predictabiliy when using this association. In fact, it's such an easy-to-reproduce scenario that I don't know how I didn't notice that before, after years of developing with Rails.

First, when things work the way we expect

Attention: the following examples that I'll use are unlikely scenarios that are not expected to happen on most applications. In case it happens, it's most likely because there is a bug in place. We should anyway be prepared for it.

Suppose you have the following in your Rails application:

create_table :products do |t|
  t.string :title
end

create_table :reviews do |t|
  t.references :product
  t.text :comment
end

class Product < ApplicationRercord
  has_many :reviews
end

class Review < ApplicationRecord
  belongs_to :product
end

Suppose now that someone just removed the reviews.product_id column but forgot to remove the related has_many :reviews association from Product. The next time we hit product.reviews, what it should happen? If you're expecting an error to be raised, you got it right.

product.reviews
# => ActiveRecord::StatementInvalid ([db_specific_error_class]: no such column: reviews.product_id)

Moreover, suppose that, instead of removing just the FK column from reviews, the whole table is removed along with the model class. In this case, an error is still raised, although different, since the association's target class definition cannot be found.

product.reviews
# => NameError (uninitialized constant Product::Review)

These two behaviors help a lot on tracking issues in the app, pointing remaining pieces of code that were forgotten somehow in the process of removing a feature from the app. The next time these pieces of code are executed, a big error would pop up in the error monitoring tool and would hardly slip under the radar.

Then, things unexpected happen

Let's go back to the first scenario where the reviews.product_id column was removed. Now suppose that the assocation belongs_to :product in Review was not removed (very unlikely, but still). What would then happen the next time the we hit review.product? If you like consistency and guessed an error, you got that wrong, unfortunately.

review.product # => nil

That's right. It just silently returns nil. The same nil that would be returned if the product_id column were still there but empty.

It gets even more interesting in the scenario where the whole table/model products/Product is removed. What happens now with review.product? If you're smart and noticed where this going and guessed nil, you got it right.

Product.take # => NameError (uninitialized constant Product)

review.product # => nil

Super weird. By returning nil, we would never be able to spot these issues early. They would pass by silently and days could go by producing unexpected results until someone notices something is wrong. It also just does not make sense at all. At least for me. If the underlying DB column does not exist, why would the association still be returning nil? We could just easily pollute an entire model with fake associations without any consequences.

class Review < ApplicationRecord
  belongs_to :i_dont
  belongs_to :care
  belongs_to :about
  belongs_to :valid
  belongs_to :associations
end

%i[i_dont care about valid associations].map do |association|
  review.send association
end
# => [nil, nil, nil, nil, nil]

In my opinion this goes very much against the robustness of active_record itself and all its ways to keep data consistent and valid, acting as a mirrored version of the DB schema on steroids, to keep wrong and incorrect data out.

Where's the issue then?

Assuming active_record latest version: 6.0.1

Digging deeply into active_record's source code, there is this method find_target? in the BelongsToAssociation:

def find_target?
  !loaded? && foreign_key_present? && klass
end

Its sole purpose is to check whether the target association needs to be found and loaded given the current conditions. This method is overriden in this class, meaning that the other associations use a more abstract and slightly different version to check if the target association must be loaded.

From the code, the target belongs_to association is only loaded if it's not yet loaded AND if the foreign key is present AND if the association class is defined.

The problem is that foreign_key_present? only checks whether the attribute is present. For sake of simplification, it would be something like @foo_id.present?. It returns false either when the FK column exists but the value is empty or if the FK does not exist at all, since no verification to check whether the column exists is being made. The condition returns eagerly with false and the target association is never tried to be loaded, returning nil in the very end. That's why also it returns nil even when class definitions are missing, since the klass part of the condition is never evaluated and the error of uninitialized constant Foo is never raised.

By simply switching the condition to !loaded? && klass && foreign_key_present?, we partially solve the problem.

I opened a WIP pull request to trigger the discussion and check whether this behavior is intentionally and by design, since after I fixed this issue locally, a bunch of unit tests failed. I realized then that there are a lot of tests relying on dynamic belongs_to associations lacking the corresponding FKs in the test suite.

I couldn't find no other information across the web depicting this issue. Let me know in the comments if you have thoughts or more information with reasons why this is the way it is.

Asserting HTTP responses with Ruby at runtime

Daniel Ferraz — Sat, 02 Nov 2019 23:42:42 +0000

I'll begin this article with a question. How do you test the HTTP requests generated by your app?

There are many approaches that can come to mind. All of them have their pros and cons.

Most of them rely on totally stubbing the HTTP calls to the external services and returning fake and controlled responses. With that, we make sure that tests are performed in isolation and never hit the real external APIs.

These are important points to consider, since reaching for the real APIs is hardly a good idea. It can make your tests slow and flaky, depending on availability of external services. Also, by stubbing the requests with fake responses, it's easy to test different happy and unhappy paths by easily switching the fake responses being used in each of your test scenarios.

Other cons of having real HTTP calls on tests are (taken from here):

Flaky tests due to connectivity issues.
Dramatically slower test suites.
Hitting API rate limits on 3rd party sites.

There are many gems available to help you avoiding making real requests, like webmock, vcr, fakeweb, no matter if you're using Rspec or Minitest.

Another good pattern which we should always do when using an external API in our code is to wrap the calls in a FooClient class, which has the sole responsability of knowing how the API is used, what are the request parameters and their formats, how to parse the responses, etc. So everything regarding the API specs are well encapsulated in one single class. Normally, this class will be directly linked to the API's official documentation and will change following the API changes. Example:

##
# Encapsulates the HTTP requests to Acme Corp. official API.
#
# For more info please refer to https://url.to.acme.api.documentaion
class AcmeAPI::Client
  def products
    response = get("#{BASE_URL}/products", default_headers)

    # Parses the response and returns it, ideally in an easy-to-use
    # object like an array of AcmeAPI::Product or something else.
  end
end

With this abstraction, when unit testing a consumer of this client object, if you're using good practices like dependency injection, it's easy to provide in the tests a stubbed FooClient instance which provides controlled responses and allows different scenarios to be tested in isolation.

describe Coyote do
  let(:coyote) { Coyote.new(acme: acme_client) }
  let(:acme_client) { AcmeApi::Client.new }

  before { acme_client.stubs(:products).returns(%w[dynamite spikes]) }

  it 'sets up a trap successfully' do
    assert_instance_of Trap, coyote.set_trap
  end

  context 'when Acme does not have any products' do
    before { acme_client.stubs(:products).returns(%w[]) }

    it 'raises an error' do
      assert_raises(LackOfResourcesError) { coyote.set_trap }
    end
  end
end

Normally, well-used 3rd party services already provide their own SDKs in different programming languages, abstracting away all the API calls and responses. In this case, you would only need to add the gem and use stubbed versions of the SDK classes in the tests.

"Ok, but what about asserting at runtime? You wrote it in your article's title, right?"

You're right. But first I had to set the ground and describe the most used approaches when it comes to test code when external API calls are involved.

The reason I asked "How do you test the HTTP requests generated by your app?" it's because there is some debate on whether the real API should be tested at all in your automated tests.

Normally you would do that in an end-to-end integration test. Since these tests are slow, you could mark them in a way that they are not always executed, but only when explicitally so. You could do that with Rspec for example, my marking them with a special type: :integration tag and running them not so often. Maybe just a few times in your CI pipeline to make sure that the API your code is using and the response it relyies didn't change suddenly and everything should be working normally. Something that the quick and stubbed tests wouldn't be able to catch, since they are relying on pre-defined responses you came up based on the documentation checked at the time of implementation.

But since these integration tests are reaching for the real API, they would still present the same problems mentioned in the beginning of this post, like flakiness, accessility issues, etc.

What to do then to catch possible integration issues as early as possible?

Testing at runtime

Let's bring back the previous Acme example. Suppose we have an additional endpoint where the Coyote fetches the product details in order to read the user manual and to know how use it before setting up a trap.

When we run the application, we would see the following output.

The coyote reached for the Acme API to fetch a product, in this case a dynamite, then fetched the product manual and read it before setting up the trap.

One simple approach I'm very happy with is to assert the responses from the API whenever they are performed during runtime.

So in the Acme API Client, when fetching for a product's details, we would assert that the user_manual is there, since this is required by the app.

def product_details(product_name)
  response = get!(
    url: "#{BASE_URL}/product_details/#{product_name}",
    headers: default_headers,
    expects: %i[user_manual]
  )

  # process the response and return
end

private

def get!(url:, headers:, expects: {})
  http_response = RestClient.get(url, headers)
  json_response = JSON.parse(http_response.body)

  check_expectations!(json_response, expects)

  json_response
end

def check_expectations!(json_response, expectations)
  expectations.each do |expected_attr|
    next unless json_response[expected_attr.to_s].nil?

    raise ExpectationError, expected_attr
  end
end

This makes sure that, whenever we hit the Products API to fetch the details of a specific product, apart from other attributes that might be present in the response, we assert that user_manual is included, since this is the data we're interested in.

In case the attribute is not present, we fail loud with an error. This adds an additional safety check in case the API changes unexpectedly and this data is not there or the attribute key changed, etc.

You might ask what benefit does this bring, since even without this additional expectations check, if the data is not there, an error would be raised anyway, most problably down the road by whoever is making use of this API client's response.

This is the catch and the issue is in that exact last sentence: most probably and down the road.

If we don't have that check, we have to make sure that, whoever is using the Acme client, be the Coyote class or any other, diligently check that the response returned from the client is in a good state and raise the approptiate error in case the data is not there. This can be very brittle.

Suppose that the API changed unexpectedly and the user_manual is now being returned as user_guide instead (bad job of Acme dev team on failing to maintain a consistent API spec).

If Coyote is diligently checking the data it got back in the response, it could raise an error when it sees that the user manual is not there and we would catch that in the bug reporting tool. The problem is that we would see this error being raised in the Coyote class and would need to track down of what went wrong, hopefully finding that the problem was in the request's response.

The image above represents what the error could look like. The stack trace is small and still relatively easy to track down the culprit. Since Coyote is the direct consumer of the API client, this is not a big issue. But can be particularly annoying if this data is only being passing forward to other objects, making the error being raised much farther down the road. Since more indirections are involved, the stack trace would be bigger and tracking down the root cause would be a bit harder.

Another more serious problem would be in the case this user_manual data is not being checked in case it gets back empty. If the Coyote (or whoever is processing this information) realizes that this user_manual is empty and assumes it's because the product does not have any user manual set up, this mean that we now have a silent bug happening in our application. A wrong assumption is being made that the product does not have user manual and the app is behaving on top of this assumption, even though the user manual exists, it's only being returned as user_guide instead.

The image below represents the Coyote making the wrong assumption about dynamites and using it without ever reading the user manual!

What the expectation check step in the Client class gives us is the guarantee that it'll fail in case anything that we expect to be there is missing. By failing loud with a nice error message, we know what went wrong and where it happened. We can then act quick and make sure to update the Client implementation to conform with the new state of the API specs.

It's also very nicely encapsulated in the API Client class, which is the one responsible to know, not only the requests URLs and parameters, but also which response attributes are supposed to be there, since these are all part from the API specifications.

The example in this article was kept simple on purpose. We're only checking the attributes in the JSON response in the first layer. But it can be easily extended to check nested attributes, array responses, not only the presence of attributes but also formats, etc.

Not to forget that this is just another layer of checking the API integration and its correct usage. It does not mean that the isolated unit tests and possibly any end-to-end test (as mentioned in the beginning of the post) should cease to exist. Theye are all suppose to work alongside each other.

Does that approach make sense to you? For me it's working great and I was able to catch errors beautifully in my bug reporting tool. Let me know in the comments.

Note: you can check the Coyote App example used in this article if it helps. Just make sure to read the README :)

Code that tries to be too clever

Daniel Ferraz — Wed, 16 Oct 2019 20:59:51 +0000

Often, when writing software, we find ourselves trying to do too much behind the scenes, without thinking if we really need our code to be that clever. This can be a problem especially on user-facing applications, like web and mobile apps. In most of the cases, these bits of business logic being handled way too smartly will eventually lead to a lot of confusion when using the app.

I find this problem particular common when dealing with web apps using Ruby on Rails. In frameworks like Rails, it’s common to accept that the framework will do too much for us “automagically” without us having to worry about what and how it’s being done. Don’t get me wrong, I find this very useful in most cases and it’s one of the main reasons why Rails got so successful and how it just makes so easy to bootstrap and MVP a new web application, with so much being done by the framework behind the scenes.

But there’s a catch. For us developers working with Rails after a reasonable amount of time, this idea of clever and magic code get so trivial and accepted in our minds, that it’s very easy to transport and leak those same patterns to our user-facing applications unconsciously, without stopping for a moment and questioning: “Wait, should I be really doing it like this? It looks clever and useful… but at what cost?”

The problem with code too clever is that, when it leads to confusion (which in most cases it does eventually), you will have a particular angry user on the other side. For us developers, users of frameworks like Rails and other gems, it’s a common picture to see our angry faces when faced with these situations. But since we’re devs, we’ll just open the related source code, investigate it further, find out what’s going on, grumble something like “duh?” or “wth”, and move on with our lives. End users of our apps, on the other hand, will either get very confused and angry, drop a bad review for the app, call the support, never use the app anymore or all at once.

The simple feature request and the magic solution

Suppose you have an app to create reservation requests at medical clinics, passing a selection of required parameters, like personal data, description of the issue or illness, specialty, insurance type, special care requirements, etc. These requests can then be accepted or denied at a later stage. Suppose now that a few of the parameters have special rules, e.g., for some type of specialties, the user is supposed to mark whether the request is an emergency and give the emergency reason. For other types of specialties, the emergency parameter does not make any sense and should not be given.

John Dev is implementing this feature, particularly the part where the reservation model is created and stored, and wonders how he should approach this rule of the urgency reason being applied only for some of the specialties, but not all. He comes up with the bright idea of, in the step of creating the record, checking which specialty was given, and if the specialty is not included in the list of specialties that accept urgency, he would just reset the urgency reason back to nil and proceed with the record creation. Rails is so nice to provide these handy hooks in the model, right? He’d just need to stick another hook in the model to provide the reset behaviour.

before_create :reset_urgency_reason_if_not_accepted

private

def reset_urgency_reason_if_not_accepted
  sely.urgency_reason = nil unless specialty_accepts_urgency?
end

“This looks like the best approach”, John Dev thinks. First, it would require less complexity in the view code, by avoiding any JS handling the urgency reason field based on the selected specialty. Second, it would facilitate the user’s life, by letting the user pick and enter any data, and everything would be handled magically behind the scenes in the backend, without the user ever needing to worry about any of these nasty rules. Win, win!

Not so fast John Dev

Let’s take a closer look on what John Dev did.

He decided to hide the business logic and complexity away of the users eyes, by handling it with the handy ActiveRecord hooks that Rails provides. Actually, this is one of the patterns I see more commonly being repeated in most of the Rails apps I worked with until now.

It’s understandable and I agree it’s very tempting to solve it like that, since it’s so easy to handle these kind of situations in the provided hooks at the record level. The more experienced reader will say something like: “Hey, but everyone knows that we should keep our ActiveRecord models skinny and avoid using hooks as much as possible.”

In fact, it doesn’t matter. Even if it was abstracted away in some other class, like in its own command / service object or any other pattern. The problem is not so much where John Dev decided to hide and handle that business logic, but the fact that it is being handled without the user’s knowledge. My example made use of the hooks because it’s one the Rails features that is more often used and abused, whenever we stick any business logic we need to handle and which is associated to the record lifecycle.

The problem of the confused user

The first problem with the mentioned approach is that it leads to a lot of confusion in the usability. The user selects a specialty and, since the field for the urgency reason is still there, the user might also mark the reservation request as urgent and provide a reason. Since there is no feedback at the view telling that the urgency is only handled for only a subset of the available specialties, it would be a big surprise for the user to only realize later that the urgency that he/she entered never saw the light of day. Since everything is being handled automagically at the record creation, although the data is consistent and all the business rules are being met, the lack of transparency leaves the user blind on what really happened. The support team (if any) will be the one to take the load.

Many people will object that this is a very poor design decision for this feature, by leaving the urgency field enabled even though a specialty which does not accept urgency was selected. A smarter decision would be to handle that on the JS side and show/hide the field depending on the selected specialty, right? Well, partially. We’ll see next that this approach has also its share of problems.

The problem of the swallowed bugs

As suggested before, John Dev decides to ehnance the approach by only displaying the urgency reason field if a specialty that accepts urgency is selected, keeping it hidden otherwise. Now the user is not confused anymore and everything looks just fine. Well, until someone introduces a bug.

We all know that code changes. And changes a lot. It’s also agreeable that all projects have different test coverage quality and, even inside the same project, test coverage quality differs at the back and front-end level. It’s very easy to think that, sometime in the future, someone can just introduce a bug where the hide/show feature on the urgency field stops working. Hopefully this will be covered by tests somehow. But then, it might also not be covered by tests, especially if the project is not yet very mature. If this takes too long to be spotted by a QA session and slips through unnoticed, we’re back in the previous situation. The confusion will be back and the not-so-good news will come from the users again.

Another situation is where the show/hide feature might not be broken, but because of some very strange edge case (which we all know it happens from time to time) the urgency reason is arriving at the model creation with some data. We most likely have a bug in our system. But since this data is getting resetted at the record creation, we might never know what is the root cause of the problem, because no one will ever notice that. Until it affects another part of the code, when it might be too late.

The point is that it can feel very brittle to handle this at the view side, and bugs can go unnoticed because we’re making sure to handle buggy data and trying to keep the state consistent.

The problem of the confused co-worker

We have to think on other very particular users of the app, which are our dev colleagues. Ideally, they are not only end users of the app, but certainly users of the source code and the past design decisions. I bet that there’s hardly no greater frustration then trying to use a particular piece of code, expecting that it behaves in a way, but surprisingly discovering that something else unexpected happens. We waste so many time debugging and investigating the source code just to find the culprit.

It’s not hard to imagine that someone in the future would need to create this record for any other feature, or maybe even using it in the tests, passing specialty and urgency as parameters, only to find out that the provided urgency data is not being persisted correctly, but constantly empty, no matter what. Hopefully it would require one file to be opened to find out the answer in the hooks. But if more layers and indirections are being used, or if the reset logic is not in the hooks, but abstracted away elsewhere, we can imagine many more files being opened until the poor co-worker finally finds out what’s going on.

Less magic, more transparency

It’s very easy to conclude that the best approach here would be to handle this case with a validation at the model and let it fail in case the urgency data is set together with a specialty that does not accept urgency.

validate -> {
  errors.add(:urgency_reason, :invalid) if urgency_reason && !accepts_urgency?
}

For some, it might be the obvious decision from the beginning. But maybe because of the simplicity of this example. Sometimes it might not be that obvious when the feature is more complex. We can easily lose our way through the implementation, over-engineering and over-complicating it by trying to be too smart and too proactive when handling the edge cases.

In most of the cases, it’s better to be explicit and a little bit dumb and just fail loud in case of an unexpected combination of data or an unexpected scenario.

In this particular example, letting the user see on the page that the urgency is not valid (or should not be provided) when that particular specialty is selected, would the best way to handle it. Combining with the show/hide feature on the field would be a plus. We facilitate users’ lives and at the same time, we’re sure that, if anything goes wrong, we know that the user will see the error, hopefully with a very descriptive message, and will be able to figure out the way around it.

Sometimes though it’s not feasible to show error messages for the users, particular when it’s hard to provide them a way to recover from the situation when something goes wrong. In this case, it’s always best to fail loud instead of trying to recover and keep the consistency of the data in a smart way. In the case where we have the validations on both front and backend, if for some reason an invalid data slips through to the backend, it’s better to see a bug report with the validation error, instead of having a smart code in place handling it and having the issue go unnoticed. Both approaches keep the state correct, but with the first one we know that something went wrong and where to look for the root cause.

If all of that sounded too trivial to you and you knew from the beginning that a validation would be a better approach, I’m happy to hear that! If you don’t agree or see any other problems with the suggested approach, please let me know. Always happy to discuss.