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

Convert spam role to junk #89

Merged
merged 5 commits into from
Jul 2, 2024
Merged

Convert spam role to junk #89

merged 5 commits into from
Jul 2, 2024

Conversation

jip149
Copy link

@jip149 jip149 commented Jun 30, 2024

According to the JMAP specification, the role for the Spam/Junk mailbox should be junk. However, some servers and client incorrectly use spam role (which does not exist in the spec).

This PR modifies the Role constructor to treat the spam role as junk.

Tests have been updated and written, however, it has not yet been tested in TwakeMail (work in progress).

jip149 added 5 commits June 30, 2024 18:06
This will allow adding tests that have different expected results
This will improve compatibility with clients and servers not following
the spec
@jip149
Copy link
Author

jip149 commented Jun 30, 2024

This is somehow unrelated, but could you explain why the Role constructor (and some other classes as well) is not const?

This would avoid the equatable mixin and lessen memory usage?

See https://dartpad.dev/?id=ab64c61ac6bc10e6b0c577656da73781

@dab246
Copy link
Member

dab246 commented Jul 1, 2024

This is somehow unrelated, but could you explain why the Role constructor (and some other classes as well) is not const?

This would avoid the equatable mixin and lessen memory usage?

See https://dartpad.dev/?id=ab64c61ac6bc10e6b0c577656da73781

We use Equatable to compare two objects to see if they are the same instance. Please use identical to check whether two references are to the same object.

Please run code on dartpad to see result

import 'package:equatable/equatable.dart';

class RoleA with EquatableMixin {
  final String value;

  RoleA(value) : value = value == "spam" ? "junk" : value;

  @override
  List<Object?> get props => [value];
}

class RoleB {
  final String value;

  const RoleB(value) : value = value == "spam" ? "junk" : value;
}

void main() {
  final junkA = RoleA("junk");
  final spamA = RoleA("spam");
  const junkB = RoleB("junk");
  const spamB = RoleB("spam");

  print('junkA == spamA ${junkA == spamA}');
  print('junkB == spamB ${junkB == spamB}');
  print('identical(junkA, spamA) ${identical(junkA, spamA)}');
  print('identical(junkB, spamB) ${identical(junkB, spamB)}');
}

@hoangdat
Copy link
Member

hoangdat commented Jul 1, 2024

Hi @jip149, can you give me some examples of problem with it?
I have only one concern now, client app maybe impacted if we forced to use junk.

@jip149
Copy link
Author

jip149 commented Jul 1, 2024

@hoangdat this is related to tmail-flutter issue #2894

I'll try to test today if the patch indeed work as expected.

@dab246 Thank you for your reply. I understand how equatable and identical work. My question is, why not use compile time constants for Role objects?

Copy link
Member

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution!

@jip149
Copy link
Author

jip149 commented Jul 1, 2024

I managed to compile TwakeMail using flutter 3.16. I have not yet fully tested all Spam related functions, but I confirm the correct folder is now used with Stalwart.

I'll do some more testing with Stalwart. Can you test that it still works with JAMES?

@dab246
Copy link
Member

dab246 commented Jul 1, 2024

@dab246 Thank you for your reply. I understand how equatable and identical work. My question is, why not use compile time constants for Role objects?

We've been developing it for quite some time, and at that time we thought it might need to change its property values ​​after initialization, so we didn't set const for it. anw, If Role only holds the role of entry data from the server we should use const to improve the performance of the program.

@chibenwa
Copy link
Member

chibenwa commented Jul 1, 2024

I'll do some more testing with Stalwart. Can you test that it still works with JAMES?

Yes, still work.

@jip149
Copy link
Author

jip149 commented Jul 1, 2024

@dab246 Thank you for your reply. I understand how equatable and identical work. My question is, why not use compile time constants for Role objects?

We've been developing it for quite some time, and at that time we thought it might need to change its property values ​​after initialization, so we didn't set const for it. anw, If Role only holds the role of entry data from the server we should use const to improve the performance of the program.

Thanks, that makes sense. What I said was actually a mistake: since Role are also created from network data, they can't be compile time constant from a const constructor. It's still possible to have all Role('something') objects be identical (and thus reduce memory usage) by a using factory constructor.

Thanks again for your patience and explanations, it's much more clearer now.

@hoangdat hoangdat merged commit 1e7d1e2 into linagora:master Jul 2, 2024
1 check passed
@hoangdat
Copy link
Member

hoangdat commented Jul 2, 2024

Hi, @jip149 , we had one side effect

tmail had the query to server like that:

image

With your change, tmail-client failed to get the good response from James (James does not aware junk)

@jip149
Copy link
Author

jip149 commented Jul 2, 2024

Hello @hoangdat!

Thank you very much for your feedback. Can you tell me more precisely what doesn't work and how I can reproduce the problem?

I suspect the problem comes from the capital 'S' at the beginning of Spam. According to the spec, the role must be lowercase. Are you sure this was working before?

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.

None yet

4 participants