Skip to content

Preserve caller fetch modifiers when adding keyword options - #746

Merged
nevans merged 2 commits into
ruby:masterfrom
OskarEichler:codex/modifiers-imap
Aug 29, 2026
Merged

Preserve caller fetch modifiers when adding keyword options#746
nevans merged 2 commits into
ruby:masterfrom
OskarEichler:codex/modifiers-imap

Conversation

@OskarEichler

Copy link
Copy Markdown
Contributor

Summary

Copy a supplied fetch modifier array before appending PARTIAL or CHANGEDSINCE keyword options. Reusing the same array currently accumulates duplicate modifiers, and frozen arrays raise FrozenError. Calls without keyword additions retain the existing allocation behavior.

Reproduction

require 'net/imap'
imap = Net::IMAP.allocate
def imap.send_command_returning_fetch_results(*args) = args
modifiers = ['VANISHED'].freeze
p imap.uid_fetch(1..3, 'UID', modifiers, changedsince: 9).last
# Before: FrozenError. After: ["VANISHED", "CHANGEDSINCE", 9].
p modifiers # still ["VANISHED"]

Verification

  • 260 focused checks, 86 failing expectations before and zero afterward. Mutable/frozen, empty/nonempty modifiers, FETCH/UID FETCH, repeated calls, partial/changedsince separately and together, zero modseq and invalid keyword inputs are covered.
  • Existing rake test on this isolated branch: 1726 tests, 12599 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications, Ruby 4.0.6 via rbenv. Baseline also passes 1,726 tests; assertion counts vary slightly between runs.
  • Supplemental RuboCop Lint retains the same 48 existing findings. Syntax and git diff --check pass. No new/modified repository tests, dependencies or workflows; focused checks were external under the consumer repository's no-new-tests policy.
  • Based on master 6d2ef7a636a1e2449187a83b06ac7a5baa54ead2; runtime differs from released 0.6.6 only in documentation before this change. Existing upstream PR searches found no matching fix.

Compatibility and limits

No signature, version or dependency change. Caller modifier arrays are no longer modified by keyword additions, including on validation errors. Only a shallow copy is needed; contained modifier objects are not changed. The reproduction stubs only command dispatch to stay offline. No production or external IMAP service used. Other Ruby/OS versions were not run locally. Local success does not imply upstream CI approval or exhaustive coverage.

@nevans

nevans commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

I'm on the fence about this one. On the one hand, the current behavior is potentially dangerous and unexpected.

On the other hand, the mod parameter to #fetch is undocumented, has never been documented, and is unsupported. It's currently needed for sending unsupported fetch extension modifiers, but my plan was to add a "proper" supported extension mechanism to fetch (and several other commands), and remove the mod argument in v0.7.0.

So I don't want to do anything to draw attention to it or encourage it's use. But it is kind of a bug.

@nevans nevans left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since mod is unsupported and I want to remove it anyway, this doesn't need to add a new test.

But, lets not explicitly check against the two supported modifiers, because that requires extra diligence in keeping this up-to-date as support for other modifiers is added

Comment thread lib/net/imap.rb Outdated
Lets not explicitly check against the two supported modifiers, because that requires extra diligence in keeping this up-to-date as support for other modifiers is added.
@nevans

nevans commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

I'm not even going to classify this as a bugfix (for the release notes), since mod is undocumented and has always been undocumented. Therefore its exact behavior is unsupported and unspecified. Use at your own risk (but it's going away soon).

@nevans
nevans merged commit 4bd4307 into ruby:master Aug 29, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants