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

Merge support for COMPRESS extension #342

Closed
wants to merge 1 commit into from

Conversation

foxcpp
Copy link
Collaborator

@foxcpp foxcpp commented Mar 4, 2020

First round of changes for #322.

  • APPENDLIMIT
  • COMPRESS
  • ENABLE
  • ID
  • SPECIAL-USE
  • UNSELECT
  • CHILDREN
  • I18NLEVEL (perhaps level 1 only?)

This PR does not include server-side change to the UNSELECT command required by #341 (needs rebase after #341 gets merged)

@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

Merging #342 into master will decrease coverage by 1.55%.
The diff coverage is 10.1%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #342      +/-   ##
=========================================
- Coverage   73.65%   72.1%   -1.56%     
=========================================
  Files          32      33       +1     
  Lines        3519    3617      +98     
=========================================
+ Hits         2592    2608      +16     
- Misses        638     715      +77     
- Partials      289     294       +5
Impacted Files Coverage Δ
client/cmd_selected.go 73.59% <ø> (ø) ⬆️
imap.go 0% <ø> (ø) ⬆️
client/client.go 66.14% <ø> (+2.17%) ⬆️
client/cmd_any.go 39.28% <0%> (-18.61%) ⬇️
server/cmd_auth.go 72.14% <0%> (-7.39%) ⬇️
mailbox.go 79.46% <0%> (-2.95%) ⬇️
deflate.go 0% <0%> (ø)
server/cmd_any.go 46.42% <0%> (-30.05%) ⬇️
server/server.go 50.53% <0%> (-0.55%) ⬇️
server/conn.go 70.33% <38.46%> (-2.26%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af3db55...8ff7eb2. Read the comment docs.

@foxcpp
Copy link
Collaborator Author

foxcpp commented Mar 4, 2020

On I18NLEVEL=1 vs 1+2 support: Client support for I18NLEVEL altogether is not very impressive according to DCS and nobody seems to care about I18NLEVEL=2.

@foxcpp foxcpp linked an issue Mar 4, 2020 that may be closed by this pull request
@foxcpp
Copy link
Collaborator Author

foxcpp commented Mar 7, 2020

@emersion What's your opinion on ID extension? Not worth merging?

@@ -30,7 +30,7 @@ func testServerTLS(t *testing.T) (s *server.Server, c net.Conn, scanner *bufio.S

io.WriteString(c, "a001 CAPABILITY\r\n")
scanner.Scan()
if scanner.Text() != "* CAPABILITY IMAP4rev1 LITERAL+ SASL-IR STARTTLS LOGINDISABLED" {
if scanner.Text() != "* CAPABILITY IMAP4rev1 LITERAL+ SASL-IR SPECIAL-USE CHILDREN UNSELECT COMPRESS=DEFLATE APPENDLIMIT STARTTLS LOGINDISABLED" {
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we can add constants for these? So that we only need to update them once when adding a new extension?

@emersion
Copy link
Owner

When this is merged, we need to create issues if the client or server side of an extension is missing.

@emersion
Copy link
Owner

What's your opinion on ID extension? Not worth merging?

Yeah, I'd prefer to leave it out if possible.

@foxcpp
Copy link
Collaborator Author

foxcpp commented Mar 12, 2020

Not sure what is better for I18NLEVEL. Is just having a variable Server.EnableI18N enough or do we want to check backend I18N "awareness" using some interface like I18NSupported()

@foxcpp
Copy link
Collaborator Author

foxcpp commented Mar 16, 2020

Oh, you know, lets get that merged and then I will continue working on it when I will have more time.

@foxcpp foxcpp marked this pull request as ready for review March 16, 2020 21:05
backend/appendlimit.go Outdated Show resolved Hide resolved
client/cmd_auth.go Outdated Show resolved Hide resolved
deflate.go Outdated
//
// It is used by COMPRESS extension implementation and it not a part of the
// library API. Do not use it.
func CreateDeflateConn(c net.Conn, level int) (net.Conn, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Should we expose this to users? Or should we define this in an internal package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not seeing a reason to keep it public.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

backend/appendlimit.go Outdated Show resolved Hide resolved
@foxcpp foxcpp mentioned this pull request Jul 27, 2020
@foxcpp foxcpp mentioned this pull request Aug 24, 2020
@emersion
Copy link
Owner

17:39 @emersion foxcpp: sorry if we've already discussed about that, but do we want to unconditionally enable the SPECIAL-USE and CHILDREN extensions?
17:39 @emersion we don't want to advertise it if the backend doesn't properly sets those attrs

@foxcpp
Copy link
Collaborator Author

foxcpp commented Aug 28, 2020

Advertising SPECIAL-USE without backend support is valid - it will just look like there MAY be special-use mailboxes but there are currently none. CREATE-SPECIAL-USE is a separate capability.
CHILDREN may be a problem though.

@emersion
Copy link
Owner

OK that makes sense

@foxcpp
Copy link
Collaborator Author

foxcpp commented Aug 28, 2020

RFC 3348 seems to be pretty lax regarding attributes too:

The CHILDREN extension defines two new attributes that MAY be
returned within a LIST response.

So we probably can get away with it without bothering much.

@emersion
Copy link
Owner

emersion commented Nov 1, 2020

All right. I rejiggered and pushed these commits:

@emersion
Copy link
Owner

emersion commented Nov 1, 2020

Holding off this PR for now because merging it would break the API: some extensions might get added twice (built-in + external).

@foxcpp
Copy link
Collaborator Author

foxcpp commented Nov 2, 2020

What is the effect of adding an external extension when it is already supported directly? Can we add a compatibility hack that would just ignore the external extension in this case?

@emersion
Copy link
Owner

emersion commented Nov 2, 2020

I wonder if it's better to let external extensions overwrite built-in extensions (and de-dup our caps), or make if it's better to make EnableExtension a no-op if already enabled (might break the API?).

@foxcpp
Copy link
Collaborator Author

foxcpp commented Nov 2, 2020

I wonder if it's better to let external extensions overwrite built-in extensions (and de-dup our caps)

The idea with some builtin extensions is that they are builtin because their support requires special changes to go-imap 'core' source code. If we let external extensions override such changes we need to have a whole bunch of if all around go-imap to disable corresponding changes and let the external extension do its thing (possibly incorrectly).

Along the way, we considered phasing out the extension interface altogether or reworking it completely for go-imap v2. I don't remember us arriving at some consensus regarding this though.

or make if it's better to make EnableExtension a no-op if already enabled (might break the API?)

It will break API with some extensions so some careful judgement is needed to figure out what we can merge into v1. Some breakage may be acceptable, for example merging https://github.com/foxcpp/go-imap-namespace is probably okay - it will prevent backends from advertising custom namespaces but I do not expect this to be used often and it is unlikely to cause any real breakage e.g. for clients.

@CrawX
Copy link

CrawX commented Nov 3, 2020

Even after merging emersion/go-imap-compress#6, COMPRESS doesn't work for me because the reader thread reads compressed data without the deflater in place which messes up the connection state completely. I found a workaround (see this and this. With these two additional changes, COMPRESS seems to actually work for me :) Does it make sense to try to get them merged? I feel like #342 might also address this race condition issue.

In any case, please include the raw-string fix from emersion/go-imap-compress#6.

@emersion
Copy link
Owner

emersion commented Nov 7, 2020

OK, so for v1, you think it's best to ignore EnableExtension calls if we already have that extension enabled? This sounds like a reasonable workaround.

@emersion
Copy link
Owner

emersion commented Nov 7, 2020

@CrawX That's weird because e.g. STARTTLS works correctly... In any case it's unrelated to this PR, so please open a separate issue about it.

@emersion
Copy link
Owner

emersion commented Sep 7, 2021

Merged UNSELECT and APPENDLIMIT. Not sure COMPRESS is worth the complexity…

@emersion emersion changed the title Merge support for small extensions Merge support for COMPRESS extension Sep 7, 2021
@emersion emersion changed the base branch from master to v1 April 14, 2023 10:27
@emersion
Copy link
Owner

Superseded by #606

@emersion emersion closed this Apr 14, 2024
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.

Implement IMAP Child Mailbox Extension
3 participants