Stop asking for a postal code on every check-in

backend/app/services/checkin/updater.rb

@@ -25,26 +25,57 @@ def initialize(current_user, params)
   end
 
   def update!
-    checkin.update!(permitted_params.except(:postal_code))
+    position = requested_position
+    update_params = permitted_params.except(:postal_code)
+
+    if location_requested?
+      if position.persisted?
+        update_params[:position_id] = position.id
+        if position.id != checkin.position_id || update_params.key?(:weather_id)
+          update_params[:weather_id] = matching_weather_id(update_params[:weather_id], position.id)
+        end
+      else
+        # A rejected replacement must not detach weather that still belongs to
+        # the existing, valid check-in location.
+        update_params = update_params.except(:weather_id)
+      end
+    elsif update_params.key?(:weather_id)
+      update_params[:weather_id] = matching_weather_id(update_params[:weather_id], checkin.position_id)
+    end
+
+    checkin.update!(update_params)
 
     if checkin.date.today?
       save_most_recent_doses
       save_most_recent_trackables_positions
     end
     update_trackable_usages
 
-    position = Position.find_or_create_by(postal_code: permitted_params[:postal_code])
-
-    if position.persisted?
-      checkin.position_id = position.id
-      checkin.save!
-    end
-
     checkin
   end
 
   private
 
+  def location_requested?
+    permitted_params.key?(:postal_code)
+  end
+
+  def requested_position
+    return unless location_requested?
+
+    Position.find_or_create_by(postal_code: permitted_params[:postal_code])
+  end
+
+  def matching_weather_id(weather_id, position_id)
+    return if weather_id.blank?
+
+    Weather.find_by(
+      id: weather_id,
+      position_id: position_id,
+      date: checkin.date.to_date
+    )&.id
+  end
+
   def update_trackables_positions(params)
     %w[Condition Symptom Treatment].each do |trackable_class_name|
       update_trackables_positions_on_destroy(trackable_class_name, params)

backend/app/services/weather_retriever.rb

@@ -1,73 +1,173 @@
+require "digest"
+
 class WeatherRetriever
+  FORECAST_MISS_TTL = 5.minutes
+
   class << self
     def get(date, postal_code)
-      position = Position.find_or_create_by(postal_code: postal_code)
+      date = date.to_date
+      position = find_or_create_position(postal_code)
 
-      weather = Weather.find_by(date: date, position_id: position&.id)
+      if position.persisted?
+        weather = Weather.find_by(date: date, position_id: position.id)
 
-      return weather if weather.present?
+        return weather if weather.present?
+      end
 
       if position&.latitude.blank? || position&.longitude.blank?
-        Rails.logger.warn "No coordinates found for postal_code #{postal_code}: #{position.inspect}"
+        Rails.logger.warn "No coordinates found for weather position"
 
         return
       end
 
-      forecast = get_forecast(date, position)
+      return if forecast_miss_cached?(date, position.id)
 
-      if forecast.status != 200
-        Rails.logger.warn "No forecast found for position #{position.inspect}: response code was #{forecast.status}, headers were #{forecast.headers}, body contained #{forecast.body}"
+      # This row lock is the short-term concurrency guard for a cache fill. The
+      # existing unique index on (date, postal_code) cannot arbitrate these writes
+      # because current records are keyed by position_id and leave postal_code nil.
+      # Long term, deduplicate existing rows and replace it with a unique index on
+      # (date, position_id), which will enforce the invariant for every writer.
+      position.with_lock do
+        weather = Weather.find_by(date: date, position_id: position.id)
 
-        return
-      end
+        return weather if weather.present?
+        return if forecast_miss_cached?(date, position.id)
+
+        if historical_date?(date, position)
+          Rails.logger.warn "No forecast for #{date} at position #{position.id}: the date is before the position's current day"
+
+          return
+        end
+
+        forecast = get_forecast(position)
+
+        if forecast.status != 200
+          Rails.logger.warn "No forecast found for position #{position.id}: response code was #{forecast.status}"
+          cache_forecast_miss(date, position.id)
 
-      create_weather(forecast, position.id)
+          return
+        end
+
+        day = daily_forecast_on(forecast, date, position)
+
+        if day.blank?
+          Rails.logger.warn "No forecast for #{date} at position #{position.id}: the date is outside the forecast window"
+          cache_forecast_miss(date, position.id)
+
+          return
+        end
+
+        create_weather(day, date, position.id)
+      end
     end
 
     private
 
-    def get_forecast(date, position)
+    # Position has no unique postal_code index, so a row lock cannot protect the
+    # instant before that row exists. A transaction-scoped advisory lock on a
+    # one-way location hash makes first creation converge on one row without
+    # putting the submitted address in SQL logs.
+    def find_or_create_position(postal_code)
+      Position.transaction(requires_new: true) do
+        lock_id = Digest::SHA256.digest(postal_code.to_s).unpack1("q>")
+        Position.connection.execute("SELECT pg_advisory_xact_lock(#{lock_id})")
+        Position.find_or_create_by(postal_code: postal_code)
+      end
+    end
+
+    def get_forecast(position)
       Tomorrowiorb.forecast(
         "#{position.latitude},#{position.longitude}",
         ["1d"],
         "imperial"
       )
     end
 
-    def create_weather(forecast, position_id)
-      today = JSON.parse(forecast.body, symbolize_names: true).dig(:timelines, :daily, 0)
-      the_time = today.dig(:time)
-      today = today.dig(:values)
-      rain_intensity = today.dig(:rainIntensityAvg)
-      sleet_intensity = today.dig(:sleetIntensityAvg)
-      snow_intensity = today.dig(:snowIntensityAvg)
-      icon = get_icon_legacy(today)
-      summary = "General conditions are #{icon}, with an average temperature of #{today[:temperatureAvg]}."
-      Weather.create(
-        date: Date.strptime(the_time, "%Y-%m-%d"),
-        humidity: today.dig(:humidityAvg).round,
+    # The forecast endpoint takes no date: it always answers with a daily timeline
+    # starting at the position's today. Pick out the day that was actually asked
+    # for -- comparing dates in the position's own time zone, since the timeline
+    # stamps each day in UTC -- so that the record we store is keyed by the date
+    # the caller wanted and the cache above can find it again. Days outside the
+    # window (a back-filled check-in, say) have no forecast to store.
+    def daily_forecast_on(forecast, date, position)
+      daily = JSON.parse(forecast.body, symbolize_names: true).dig(:timelines, :daily) || []
+      time_zone = time_zone_for(position)
+
+      daily.find { |day| local_date(day[:time], time_zone) == date }
+    end
+
+    def historical_date?(date, position)
+      date < Time.current.in_time_zone(time_zone_for(position)).to_date
+    end
+
+    def time_zone_for(position)
+      NearestTimeZone.to(position.latitude.to_f, position.longitude.to_f).presence || "UTC"
+    end
+
+    def forecast_miss_cached?(date, position_id)
+      Rails.cache.read(forecast_miss_cache_key(date, position_id)) == true
+    end
+
+    def cache_forecast_miss(date, position_id)
+      Rails.cache.write(
+        forecast_miss_cache_key(date, position_id),
+        true,
+        expires_in: FORECAST_MISS_TTL
+      )
+    end
+
+    def forecast_miss_cache_key(date, position_id)
+      "weather_retriever/forecast_miss/#{position_id}/#{date.iso8601}"
+    end
+
+    def local_date(time, time_zone)
+      Time.parse(time.to_s).in_time_zone(time_zone).to_date
+    rescue ArgumentError
+      nil
+    end
+
+    def create_weather(day, date, position_id)
+      values = day.dig(:values)
+      rain_intensity = values.dig(:rainIntensityAvg)
+      sleet_intensity = values.dig(:sleetIntensityAvg)
+      snow_intensity = values.dig(:snowIntensityAvg)
+      icon = get_icon_legacy(values)
+      summary = "General conditions are #{icon}, with an average temperature of #{values[:temperatureAvg]}."
+      weather = Weather.new(
+        date: date,
+        humidity: values.dig(:humidityAvg).round,
         icon: icon,
         position_id: position_id,
         precip_intensity: rain_intensity + sleet_intensity + snow_intensity,
-        pressure: today.dig(:pressureSurfaceLevelAvg),
+        pressure: values.dig(:pressureSurfaceLevelAvg),
         summary: summary,
-        temperature_max: today.dig(:temperatureMax),
-        temperature_min: today.dig(:temperatureMin)
+        temperature_max: values.dig(:temperatureMax),
+        temperature_min: values.dig(:temperatureMin)
       )
+
+      return weather if weather.save
+
+      Rails.logger.warn "Could not store weather for #{date} at position #{position_id}: #{weather.errors.full_messages.to_sentence}"
+
+      # Do not hand callers an unsaved record whose nil id would be persisted as
+      # "this check-in has no weather". The lookup also tolerates a writer that
+      # does not participate in the position-row locking protocol above.
+      Weather.find_by(date: date, position_id: position_id)
     end
 
-    def get_icon_legacy(today)
+    def get_icon_legacy(values)
       # Our icons do not coverage their full range of weather codes. We could pull in their icons (linked below) on the frontend to expand options
       # This method adapts their weather codes to our existing icons as best as possible
       # Icons and codes found here: https://docs.tomorrow.io/reference/data-layers-weather-codes
       # Icon files here: https://github.com/Tomorrow-IO-API/tomorrow-weather-codes
       # Daily forecast is always daytime weather codes / icons regardless of actual time
-      code = if today["weatherCodeMin"]
-        today["weatherCodeMin"]
-      elsif today["weatherCodeFullDay"]
-        today["weatherCodeFullDay"]
+      # The forecast body is parsed with symbolized names, so these keys are symbols
+      code = if values[:weatherCodeMin]
+        values[:weatherCodeMin]
+      elsif values[:weatherCodeFullDay]
+        values[:weatherCodeFullDay]
       else
-        today["weatherCode"]
+        values[:weatherCode]
       end
 
       case code

backend/config/initializers/filter_parameter_logging.rb

@@ -3,6 +3,12 @@
 # Configure parameters to be partially matched (e.g. passw matches password) and filtered from the log file.
 # Use this to limit dissemination of sensitive information.
 # See the ActiveSupport::ParameterFilter documentation for supported notations and behaviors.
+location_filters = [:postal_code, :latitude, :longitude]
+
 Rails.application.config.filter_parameters += [
-  :password, :passw, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn
+  :password, :passw, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn,
+  *location_filters
 ]
+
+# Active Record filters SQL bind values separately from request parameters.
+ActiveRecord::Base.filter_attributes += location_filters

backend/spec/config/filter_parameter_logging_spec.rb

@@ -0,0 +1,20 @@
+require "rails_helper"
+
+RSpec.describe "parameter filtering" do
+  it "redacts submitted locations and derived coordinates from application logs" do
+    filter = ActiveSupport::ParameterFilter.new(Rails.application.config.filter_parameters)
+
+    location = {
+      "postal_code" => "123 Main Street",
+      "latitude" => 44.967486,
+      "longitude" => -93.2897678
+    }
+
+    expect(filter.filter(location)).to eq(
+      "postal_code" => "[FILTERED]",
+      "latitude" => "[FILTERED]",
+      "longitude" => "[FILTERED]"
+    )
+    expect(Position.filter_attributes.map(&:to_s)).to include("postal_code", "latitude", "longitude")
+  end
+end

backend/spec/controllers/api/v1/weathers_controller_spec.rb

@@ -39,6 +39,17 @@
       it { is_expected.to include(*expected_keys) }
       it { is_expected.not_to include(*not_expected_keys) }
       it { expect(response).to have_http_status(:ok) }
+      it { expect(json_response[:weather][:id]).to eq(weather.id) }
+    end
+
+    describe "index when no weather is available" do
+      let(:json_response) { JSON.parse(response.body, symbolize_names: true) }
+
+      before { expect(WeatherRetriever).to receive(:get).and_return(nil) }
+      before { index_action }
+
+      it { expect(response).to have_http_status(:ok) }
+      it { expect(json_response).to eq(weathers: []) }
     end
   end
 end

backend/spec/services/checkin/creator_spec.rb

@@ -109,18 +109,37 @@
       end
     end
 
-    context "when postal code is set on previous checkin" do
-      let(:weather) { create :weather }
+    context "when a location is set on the previous checkin", :vcr do
+      # The recorded forecast is Minneapolis from 2023-12-05 onwards, so pin "today"
+      # inside that window: the trackings above are only active from today.
+      let!(:date) { Date.parse("2023-12-05") }
+      let(:cassete) { "WeatherRetriever/#{postal_code}" }
       let(:postal_code) { "55403" }
-      let(:position) { Position.create(postal_code: postal_code) }
+      let(:position) { VCR.use_cassette(cassete) { Position.create(postal_code: postal_code) } }
 
-      let!(:previous_checkin) { create :checkin, user_id: user.id, position_id: position.id }
+      let!(:previous_checkin) do
+        create :checkin, user_id: user.id, date: date - 1.day, position_id: position.id
+      end
+
+      subject { VCR.use_cassette(cassete) { Checkin::Creator.new(user.id, date).create! } }
+
+      around { |example| travel_to(date) { example.run } }
 
-      before { expect(WeatherRetriever).to receive(:get).and_return(weather) }
+      before { allow(Tomorrowiorb).to receive(:api_key).and_return("MY_MEGA_TOMORROW_IO_KEY") }
 
-      it "should ask for weather" do
+      it "carries the location over and asks for that day's weather" do
         expect(subject.position.postal_code).to eq(postal_code)
-        expect(subject.weather_id).to eq(weather.id)
+        expect(subject.weather).to be_present
+        expect(subject.weather.date).to eq(date)
+      end
+
+      context "when the weather for that date cannot be retrieved" do
+        before { allow(WeatherRetriever).to receive(:get).and_return(nil) }
+
+        it "still carries the location over" do
+          expect(subject.position.postal_code).to eq(postal_code)
+          expect(subject.weather_id).to be_nil
+        end
       end
     end
   end