Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Frozen SearchResult breaks IMAP.find with what: :last option #262

Closed
stanley90 opened this issue Feb 2, 2024 · 6 comments · Fixed by #263
Closed

Frozen SearchResult breaks IMAP.find with what: :last option #262

stanley90 opened this issue Feb 2, 2024 · 6 comments · Fixed by #263

Comments

@stanley90
Copy link

PR #236 introduced the SearchResult class, which is actually a frozen Array.
However, this breaks the what: :last option in the mail gem here
FrozenError: can't modify frozen Net::IMAP::SearchResult

nevans added a commit that referenced this issue Feb 4, 2024
This was backward incompatible with existing usage of search results:
* Fixes #262
@nevans
Copy link
Collaborator

nevans commented Feb 4, 2024

Thanks for reporting this. I should have realized that freezing by default wouldn't be backward compatible! That's fixed by #263.

I'm hoping that using the subclass is still okay though. I'd prefer not to add a config option (e.g: search_result_type => Array | SearchResult | ESearchResult) and I'd prefer to return the same type regardless of whether or not we see a MODSEQ. What do you think?

nevans added a commit that referenced this issue Feb 4, 2024
This was backward incompatible with existing usage of search results:
* Fixes #262
@stanley90
Copy link
Author

Honestly, I'm not convinced that the mail gem should just reverse! it in place, safer would be to make a copy.
Otherwise I guess an Array subclass is okay.
Btw, can I expect a release rather soon? Need to decide whether to temporarily work around this / downgrade 🙂

@nevans
Copy link
Collaborator

nevans commented Feb 4, 2024

Honestly, I'm not convinced that the mail gem should just reverse! it in place, safer would be to make a copy. Otherwise I guess an Array subclass is okay.

I would prefer that too. But backward compatibility is more important, especially since there were no deprecation warnings.

Btw, can I expect a release rather soon? Need to decide whether to temporarily work around this / downgrade 🙂

Yes, today or tomorrow. 🙂

@nevans
Copy link
Collaborator

nevans commented Feb 4, 2024

@stanley90 It's been released. Let me know if v0.4.10 works for you.

@stanley90
Copy link
Author

@stanley90 It's been released. Let me know if v0.4.10 works for you.

Works, thanks!

@PaulinaCarrilloRuiz09
Copy link

💯

nevans added a commit to nevans/mail that referenced this issue May 21, 2024
The code had several issues with existing IMAP UID search:
* net-imap <= v0.3 returns `nil` when the server returns nothing.
  (>= v0.4 returns an empty array when the server returns nothing.)
* net-imap v0.4.8 and v0.4.9 return a _frozen_ `SearchResult`.
  `SearchResult` inherits from `Array`, but because it's frozen,
  mutating it with `reverse!` results in an exception.
  See ruby/net-imap#262.
  (v0.4.10 and above doesn't freeze SearchResult.)
* newer net-imap (>= 0.5) will support servers that send `ESEARCH`
  results, but those will be returned as an `ESearchResult` struct
  (probably frozen).  `IMAP4rev1` servers shouldn't return `ESEARCH`
  unless the client has activated that extension, but `IMAP4rev2`
  servers will _always_ return `ESEARCH`.
* RFC3501 says nothing about sorting the UIDs that come back from
  `UID SEARCH`.  Almost all servers do return sorted UIDs, most of the
  time.  But it isn't 100% reliable.

The fix is simple:
* use `#to_a` to convert both `nil` and `ESearchResult` into an array.
* use `#sort` to ensure the UIDs are sorted.  This also returns a new
  array, without mutating the original (which may be frozen).
nevans added a commit to nevans/mail that referenced this issue May 21, 2024
The code had several issues with existing IMAP UID search:
* net-imap <= v0.3 returns `nil` when the server returns nothing.
  (>= v0.4 returns an empty array when the server returns nothing.)
* net-imap v0.4.8 and v0.4.9 return a _frozen_ `SearchResult`.
  `SearchResult` inherits from `Array`, but because it's frozen,
  mutating it with `reverse!` results in an exception.
  See ruby/net-imap#262.
  (v0.4.10 and above doesn't freeze SearchResult.)
* newer net-imap (>= 0.5) will support servers that send `ESEARCH`
  results, but those will be returned as an `ESearchResult` struct
  (probably frozen).  `IMAP4rev1` servers shouldn't return `ESEARCH`
  unless the client has activated that extension, but `IMAP4rev2`
  servers will _always_ return `ESEARCH`.
* RFC3501 says nothing about sorting the UIDs that come back from
  `UID SEARCH`.  Almost all servers do return sorted UIDs, most of the
  time.  But it isn't 100% reliable.

The fix is simple:
* use `#to_a` to convert both `nil` and `ESearchResult` into an array.
* use `#sort` to ensure the UIDs are sorted.  This also returns a new
  array, without mutating the original (which may be frozen).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants