[tudorpavel]

Adds support for the overrides and historical values API to the Ruby client, targeting the current API where write responses always include overrides.

• Customer.create!/update! and Contact.create!/update! accept overrides; add_custom_attributes!, update_custom_attributes!, remove_custom_attributes! and add_custom_attributes_by_email! take an overrides: keyword argument, with existing brace-less attribute calls kept working via ** reassembly — the one exception is an attribute literally named overrides on update_custom_attributes!, which now needs the braced form (pinned by a spec).
• Customer.retrieve, Contact.retrieve and retrieve_attributes (class and instance) pass the with_overrides and attributes_with_history query params; overrides and historical_values are readable on Customer and Contact.
• Overrides echoed in responses are never resent: only an explicitly assigned overrides hash is serialized, exactly once — a later update! on the same instance sends no flags, so ordinary updates cannot re-protect stale in-memory values. Echoed hashes are deep-frozen, so in-place edits raise instead of silently vanishing. The instance custom attribute helpers sync overrides[:attributes] from their responses as read-only state (a pending assigned hash is not clobbered).
• Custom attribute names under overrides and historical_values are exempt from response snake-casing (immutable keys) and round-trip verbatim — on Customer only, since contact overrides and list responses carry standard field names.
• Fixes remove_custom_attributes! assigning the whole response (custom + overrides + message) to custom_attributes; it now assigns custom and returns the full response.
• Specs: 413 examples, 0 failures; the new VCR examples match on request body (same pattern as activities_export_spec) — including class-level update! with overrides and the instance retrieve_attributes delegation — and regression specs cover the legacy call forms, the no-replay semantics, and the overrides sync.
• VCR cassettes are recorded against a real test account (TEST_API_KEY; while it is set, WebMock.allow_net_connect! enables recording) and replay offline with request-body matching.

[keith-chartmogul]

The code generally looks good to me, the only thing catching my eye being the invalid customer UUID in one of the VCR cassettes, which I left a comment on. 👀

Claude surfaced a few points as well which I added inline for your consideration. Please feel free to ignore if they are overly complicated, but I just wanted to make sure that you did not miss any of these during your development of this PR. 👍

[keith-chartmogul]

Thanks a lot for addressing all of my feedback! All of your improvements LGTM. 🙏

[keith-chartmogul]

I found some more areas for improvement, focused on testing and documentation, that I think are worth addressing since we are in here making changes. These will also make the upcoming release that much better. 💪

[keith-chartmogul]

The attribute endpoints echo the confirmed override state back (e.g. {"custom":{...},"overrides":{"custom":{"MyChannel":true}}}), but none of these three helpers use it:

• add_custom_attributes! and update_custom_attributes! keep only [:custom] and throw the echoed overrides away.
• remove_custom_attributes! returns the full response but doesn't refresh @overrides either.

So after this call, customer.overrides[:attributes] still shows the old state, and the only way to see the confirmed pin is another retrieve:

customer.add_custom_attributes!({ type: 'String', key: 'MyChannel', value: 'Facebook' },
                                overrides: { custom: { MyChannel: true } })

The return values are also inconsistent: add/update return the custom hash, while remove returns the whole response.

Could we refresh the attributes.custom subtree of @overrides from response[:overrides][:custom] in all three? Build a new hash and pass it through set_overrides so it stays frozen, and note that the echoed shape here is {custom: …} rather than {attributes: {custom: …}}. If that's more than you want to take on in this PR, a comment saying overrides is only refreshed by retrieve, create! and update! would also work.

[tudorpavel]

It's been a while and my brain got wiped free of the context on these SDKs, I think I've gone back and forth on should the server-returned truth update the in memory object?

Historically that's been true and in the case of updating pinned attributes like customer.company that's still true: if the company name is pinned the server returns the pinned value and wipes my local value. So I guess we should embrace this everywhere and have that be the convention, local objects get assigned server truth on response and c'est la vie. Do you agree this makes the most sense?

[keith-chartmogul]

Yes, I think this makes sense and I agree. 👍

[tudorpavel]

I was ready to go full gung-ho and let server-returned overrides change local state everywhere, but in the end I've let the LLMs gaslight me into keeping most of the design on how we handle overrides in requests differently but it still irks me as maybe too complex...

Anyway your point here is about sync consistency for custom attr helpers on Customer objects, which I've fixed in 1c13969.

---

# A nightly sync script holds a customer snapshot, pin state included:
customer = ChartMogul::Customer.retrieve(uuid, with_overrides: true)
customer.overrides                          # => { company: true }

# Meanwhile, a teammate decides the pin was a mistake and removes it:
#   PATCH /v1/customers/:uuid   {"overrides": {"company": false}}

# Later the script fixes a typo — it has no opinion about pins at all:
customer.name = 'Acme Inc.'
customer.update!

# If `overrides` were a plain writeable attribute, that PATCH body would be
#   {"name": "Acme Inc.", ..., "overrides": {"company": true}}
# and the API reads request overrides as pin/unpin COMMANDS, not state:
# the teammate's unpin is silently undone by a name fix, attributed to the
# script, pinning whatever value `company` holds now — one the script never saw.

# With intent tracking, the typo fix carries no overrides at all:
customer.serialize_for_write[:overrides]    # => nil

# Pins change only when the caller actually says so, and exactly once:
customer.overrides = { company: true }
customer.serialize_for_write[:overrides]    # => { company: true }
customer.update!
customer.serialize_for_write[:overrides]    # => nil   (intent consumed)

The one-sentence root cause to pair with it: in a response, overrides is a state snapshot; in a request, it's a command — and replaying a snapshot as commands undoes other actors' changes. company round-trips safely because it's a value in both directions; overrides is the one field that changes meaning with direction, so it's the one field that needs intent tracking.

[tudorpavel]

But I think I need a second opinion too: does it make sense for requests to ignore overrides unless they've been explicitly set each time? So like a sequence of two updates might be confusing that "request" overrides are cleared before the second update which is how it currently works.

# Pin at creation — one explicit command:
c = ChartMogul::Customer.create!(data_source_uuid: DS, external_id: 'seq-1',
                                 name: 'Seq Co', company: 'Acme',
                                 overrides: { company: true })
c.company    # => "Acme"      c.overrides  # => {company: true}

# Ordinary update of another field — NO overrides needed, pin persists server-side:
c.name = 'Seq Co (renamed)'
c.update!
c.company    # => "Acme"      c.overrides  # => {company: true}

# Trying to change the pinned field WITHOUT a command — server restores it:
c.company = 'Acme 2'
c.update!
c.company    # => "Acme"      (your change was discarded; object synced back)

# Changing the pinned value requires re-issuing the pin command with the new value:
c.company = 'Acme 2'
c.overrides = { company: true }
c.update!
c.company    # => "Acme 2"    c.overrides  # => {company: true}

# Removing the pin is also one command:
c.overrides = { company: false }
c.update!
c.overrides  # => {}          (company stays "Acme 2", now unprotected)

I think the right mental model is to have "request" overrides be passed as an arg to update!, all other methods in all SDKs besides Ruby SDK's instance update! have a distinction between request and response overrides so it's clearer. 🙃

[tudorpavel]

@ya-petrov-evgeniy @jessicahearn perhaps if you can spare a few minutes you can pitch in with your thoughts on the ergonomics here and how we handle request-side overrides for instance methods like customer.update!.

[keith-chartmogul]

A couple of paths that aren't covered yet:

• Class-level Customer.update!(uuid, company: 'New Co', overrides: { company: true }). Contact has a VCR example for this, but Customer only tests instance update! with stubs.
• The new instance delegation customer.retrieve_attributes(with_overrides: true, ...). Only the class method is exercised.

[tudorpavel]

Done 32b94e5 👍

[keith-chartmogul]

This example only uses title, so nothing checks the reason overrides and historical_values were added to set_immutable_keys: contact custom attribute names should come back without being snake-cased.

Could we add a custom attribute to the fixture? For example "overrides": { "custom": { "MyChannel": true } } and attributes_with_history: 'custom.MyChannel', then assert the :MyChannel key survives in both hashes.

[tudorpavel]

Ah actually we haven't yet implemented overrides and history for Contact custom attrs, so they shouldn't be in set_immutable_keys for Contact at all. Removed in 15bb2a7. And the scenario you mentioned is covered by Customer specs where it's indeed relevant.

Good instinct, but that fixture shape can't occur: contact overrides and history cover standard fields only (contact custom attribute pins were descoped — PIP-546), and attributes_with_history=custom.MyChannel 422s on the contact endpoint, so there are no case-sensitive names to protect. The real finding is that overrides/historical_values never needed to be in Contact's set_immutable_keys at all — removed them (every contact field name is already snake_case). The customer specs pin verbatim casing where it genuinely matters, on overrides[:attributes][:custom] and historical_values.

[keith-chartmogul]

Nit: as far as I can tell from the platform, /v1/customers list and search never pass with_overrides, so these keys don't show up in Customers responses today. That makes this change defensive and untested. I'm fine keeping it for consistency, but it'd be worth either a small spec or dropping it.

[tudorpavel]

Ah you're right and this is the same deal as above, they shouldn't be in plural Customers class at all, removed in f9a4e95.

Dropped it — list and search never carry these keys (same platform check as the contact thread: with_overrides is declared only on the single-record GET), and the by-email response parses through Customer's own class methods, so the singular class's immutable keys cover every real path. Matches the contact cleanup.

[keith-chartmogul]

Nit: main already has WebMock.allow_net_connect! if ENV['TEST_API_KEY'] at the bottom of this file (line 42), so this one is redundant. It also splits the require block, so I think it can go.

[tudorpavel]

Fixed 35e36db 👍

[keith-chartmogul]

Also applies to lines 78-105 in lib/chartmogul/contact.rb

Optional: overrides=, serialize_overrides, set_overrides, freeze_nested and the comment are identical in Customer and Contact. A small Concerns::Overridable in lib/chartmogul/concerns/ (next to Entries and Pageable) would stop the two copies drifting apart, and it would be the obvious hook if other resources gain overrides later. Happy to leave it as is if you'd rather not.

[tudorpavel]

Hmm I think I prefer the duplication if extracting needs to use class_eval, some duplication I think is ok in the age of LLMs. 👌

Looked into it and decided to leave the two copies. The catch: writeable_attr defines the generated writer with define_method directly on the class, so a plain module's overrides= would be shadowed by it and intent tracking would silently stop working — the concern only works as a def self.included(base) + base.class_eval block, with include order mattering relative to the attr declarations. That's enough ceremony and subtlety that the cure feels worse than the duplication: both copies are pinned by specs on each class, and if a third resource ever gains overrides we can extract then with that known cost.

# lib/chartmogul/concerns/overridable.rb
module ChartMogul
  module Concerns
    module Overridable
      # (the big comment moves here)
      def self.included(base)
        base.class_eval do
          def overrides=(value) ... end
          def serialize_overrides ... end
          private
          def set_overrides(value) ... end
          def freeze_nested(value) ... end
        end
      end
    end
  end
end

[keith-chartmogul]

Also applies to line 366 in spec/chartmogul/contact_spec.rb

Nit: request_double, stub_api_request and sent_body are defined identically in both spec files. Could they move to spec/support/ next to the existing shared examples?

[tudorpavel]

This one is more straightforward to extract, so it's done in 241f340. 👍

[keith-chartmogul]

Could we add a short comment on the public helpers covering a few things callers can currently only learn from the specs or the PR description?

• the overrides: keyword;
• the override shape for these attribute endpoints ({ custom: { Key: true } }), which differs from Customer.create!/update! ({ attributes: { custom: { Key: true } } });
• that a custom attribute literally named overrides has to use the braced form here.

[tudorpavel]

Done 87dce75, I've also had it fix mentions of internal pin/unpin terminology that slipped through in other comments.

[keith-chartmogul]

Also applies to line 78 in lib/chartmogul/contact.rb

Small addition to this comment: the echoed hash is deep-frozen and dup is shallow. So something like h = customer.overrides.dup; h[:attributes][:custom][:X] = true still raises FrozenError. Maybe mention building a fresh hash, or a deep copy, rather than dup?

[tudorpavel]

Good nit, fixed 9558224 👍 I think it reads better now.