168553 - Need to fork rules.dat

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Scott MacGregor]

With all the improvements we are making to filters, we are going to need to fork
rules.dat since the new attributes are not backwards compatible. 

Some notes: 

Navin and I were just talking about this in my cube when David made his post. We
also think the only solution is to fork rules.dat.

What about the following:

1) The next time we write out the filter rules, we write them to a new file:
"rules1.dat" (is there a better name we should use?)

2) When reading in the filters, we first look for rules1.dat, if we don't find
it we fall back to rules.dat.

In short, the first time you edit your filters with this new build we will fork
rules.dat and from that point forward trunk builds would look at the new filter
file.

Comment 1

[Navin Gupta]

Attachment: patch to fork rules.dat

My approach to fork it right away when we are getting filter list. So copy
rules.dat to rules1.dat and rules1.dat is now the default filter file. This is
much  simpler assuming scott's whitelisting stuff is going to be used heavily
as soon as it lands.  All other changes are to rename rules.dat to rules1.dat
in alert wordings and const(s).

Comment 2

[Navin Gupta]

David, Scott, Can you review ? thx. 

Comment 3

[Navin Gupta]

Can I get reviews ? thx.

Comment 4

[Navin Gupta]

Cavin, Can I get r=? thx.

Comment 5

[Cavin Song]

Comment on attachment 99168 [details] [diff] [review]
patch to fork rules.dat

r=cavin.

Comment 6

[David :Bienvenu]

Comment on attachment 99168 [details] [diff] [review]
patch to fork rules.dat

I'm unclear why the comm4x change is needed - we're not writing out rules1.dat
into directories that we're going to import 4.x files from, are we? Or do we
somehow run the import code on Mach V directories?

Comment 7

[Navin Gupta]

yes, if we are importing that change is not needed. I will not check that change
in. 

Comment 8

[David :Bienvenu]

Ok, thx.

+#define FILTER_FILE_NAME	"rules1.dat"		/* this is XP in 5.x */

could you fix this comment? I'd just say "this uses a cross-platform file
format" or something like that.

other than that, sr=bienvenu.

Comment 9

[Navin Gupta]

fix checked in. 

Comment 10

[Joe Infla]

Just a thought:  This solution seems like it could present a problem moving back
and forth between Mozilla versions while using the same profile.  Suppose I
update filters in a recent build, then return to v1.1 for daily operation.  I
won't see those new filters while using 1.1, and any filters I later add using
1.1 won't show up when I use more recent builds (because rules1.dat will have
previously been created).  

Comment 11

[Greg K.]

Might it be better to use a more descriptive filename, something that conveys
more information on what's different between "rules.dat" and "rules1.dat"?
Something like "rules-improved.dat"?

Is this a permanent change, or will things be rolled back into plain old
rules.dat at some point in the future?

Comment 12

[David :Bienvenu]

I'm a little confused - did we also checkin the changes that require that we
fork rules.dat (mscott's changes)? If not, then we're still going to have an
incompatibility going back and forth between current trunk builds that have
mscott's changes, and the builds that don't have his changes. I thought the plan
was to check in the forking when we checked in the changes that made rules.dat
incompatibile. Also, did we address the versioning issues so that we wouldn't
have to fork again?

Comment 13

[David :Bienvenu]

yes, moving back and forth will not work, in terms of having the same filters.
We did this because of a problem in the filter criteria filing code that we ran
into when we started adding new kinds of filters. The only solution we could
come up with was to fork the rules.dat file at the time when we checked in the
code that created the new kinds of filters. We were also going to change the
filing code so that it could handle problems like this in the future, so that
this would be a one-time only fork.

Comment 14

[Navin Gupta]

yes, you are right. I backed out this checkin. This needs to go with mscott's 
filter changes.

Comment 15

[Tushar Teredesai]

How about having a better name for the file, say moz-rules.dat:)

Comment 16

[David :Bienvenu]

great, thx for backing this out. I might also need to coordinate my changes for
a couple new filter actions, and Seth might as well for his new action.

We could make the name something like msgFilterRules.dat - then people will
really know what it's for!

Comment 17

[Henrik Gemal]

any chance for a better filename that rules1.dat?

why not just "mailrules.dat" or "mozrules.dat".

we now have the chance!

please...:)

Comment 18

[Frederic Bezies]

I don't know if it is related to patch backup, but I have some strange
"rules1.dat" folder in every account in mailnews ?!

Comment 19

[Alex Bishop]

> any chance for a better filename that rules1.dat?

Someone in n.p.m.mail-news suggested filters.dat, which I like.

http://groups.google.com/groups?as_umsgid=[e-mail]

Comment 20

[Greg K.]

Could it be mixed case? Filters.dat?

Comment 21

[Scott MacGregor]

Yeah we should probably use a name like:

filters.dat
or
mailFilters.dat

instead of rules1.dat (which I'm guilty of initially suggesting). Please don't
spam the bug with all the possibile names, we'll come up with something
reasonable before we check in and will post it here. 

Comment 22

[Navin Gupta]

*** Bug 176897 has been marked as a duplicate of this bug. ***

Comment 23

[(not reading, please use [e-mail] instead)]

bringing over comments from bienvenu from another bug:

"I think we need a plan for this. I guess Scott has already checked his stuff in,
though not turned on in Mozilla, so maybe the horse is out of the barn already.
But, it seems to me that what should happen is that we should turn on all the
changes that require a fork at the same time that we check in the code that does
the fork. If we check in code that will change the format of the filters file
w/o checking in the forking code, we might break the ability to go back and
forth between 7.0 and the tip of the trunk bits. And if we break people's
filters, we will make a lot of people unhappy. It's one thing for an old version
of mozilla to be unable to read a newer filter format, but another for it it to
read the newer filter format, but lose information when it writes it back out.
One simple approach going forward would be to change the filing code to read
newer format filter files, but refuse to write out the file, on the theory that
it might not have read the filter file correctly.

I realize this sounds like an enormous pain since everything would need to be
turned on at once, and some of the work hasn't started yet. In theory, we could
do just the filing code for the features that aren't done yet (I'm thinking of
the new filter actions, forward and reply), but I'm not sure exactly what file
format wil be for the data of these actions. I guess would could just say it's a
string and leave it at that."

Comment 24

[(not reading, please use [e-mail] instead)]

blocks the complete landing of the view code.  (see #176850)

Comment 25

[(not reading, please use [e-mail] instead)]

"I guess Scott has already checked his stuff in, though not turned on in
Mozilla, so maybe the horse is out of the barn already."

dmose currently owns bringing over mscott's view code (bug 176850) and turning
on the spam feature (bug 169638).

Those are two features that can be landed (but not competely turned on) until we
have a fix for this, and we have a fix for the other changes.

I agree with david and mscott: we should fork only once.

is is possible that the new format can be done in a way that will prevent future
forks?  If so, it would allow us to fork before all our changes are ready.

Otherwise, we'd have to wait to turn on these features, or fork again.

Here are some comments from mscott:

sspitzer: how much more until we fork rules.dat?
mscott: i don't know who else is doing filters work, anyone?
mscott: i don't have anything left
sspitzer: I have sound filters
sspitzer: let me ask david / navin what they have left
sspitzer: sorry to ask you again and again
sspitzer: but what requires us to fork?
sspitzer: I mean we could just make the minimal changes and fork,
sspitzer: and then finish the rest of filters features later?
mscott: then you might have to fork again if you make more changes 
mscott: later that cause another change to how a filter gets written out
mscott: just do one fork and not a fork today, another one next week
sspitzer: I mean can we do this:
sspitzer: right, one fork, but say I need to add sound filter stuff
sspitzer: so add the changes we need to avoid a second fork
sspitzer: but don't implement the whole thing.
sspitzer: does that make sense?
mscott: sure you can do that if you know what you need to add...
mscott: we don't know what that is.
mscott: at least i didn't for my stuff
sspitzer: good point.

Comment 26

[(not reading, please use [e-mail] instead)]

other way around, this can't land until 169638, 176850 lands (or we figure out
how to fork, and avoid future forks.)

Comment 27

[(not reading, please use [e-mail] instead)]

adding bug 118952, adding support for sound filter actions.

a list of features that might block this bug:

1) Multiple Filter actions
2) Reply/Forward/Stop Filter Execution filter actions
3) Play Sound filter action
4) filter by addressbook UI (might be addressed by the view bug)
5) filter by charset (or filter by non-ascii)

Comment 28

[scottputterman]

Is there anything we can do to make it so we don't have to fork again the next
time we make changes to filters?  Can we make it so that the trunk can handle
backwards compatibility?

Comment 29

[David :Bienvenu]

I suggested the following in a previous comment:

"One simple approach going forward would be to change the filing code to read
newer format filter files, but refuse to write out the file, on the theory that
it might not have read the filter file correctly."

What tends to happen is that we add new filter attributes, and it doesn't tend
to need to break the filing input code, which could just ignore stuff it doesn't
understand.

So the way it would work is as follows:

1. Change the filing code to ignore attributes it doesn't recognize (and skip to
the next line).
2. Make sure that we ignore filter actions and action values we don't understand
(and probably disable the filter in memory).
3. Note that the file version is a newer version than we understand
4. If the user tries to bring up the filter editor, put up a message that says
something like "this filter file was created by a newer version of
Netscape/Mozilla. You can only edit the filter file with that version of the
product".
5. Make sure that no other code (besides the filter editor) causes the filter
file to get written out, in the case that we're editing a newer version than we
understand.

This isn't a fool proof approach. But I think it would allow us to add some new
kinds of filters w/o forking rules.dat.

Comment 30

[scottputterman]

Is there anyway we could make it so that it disables filters it can't understand
but still retains the filter so that filter changes could be made in both versions?

Comment 31

[David :Bienvenu]

sure, we could switch over to using mork for the filter filing :-) I'm half
serious. We'd need what mork provides, which is the ability to have the
db/client code ignore but retain information it doesn't understand. The mork
filing code is completely format agnostic - there are basically tables, rows,
and cells, and it's up to the client code to interpret the rows and cells. The
problem with the filter filing code (which I wrote), is that the filing format
and the filter data structures are too tightly bound together. 

Given that we probably won't switch over to Mork, we'd need to extend the filter
object so that it will read in and remember lines it doesn't understand, and
file those lines out when asked to file itself out. This actually wouldn't be
that hard.

Comment 32

[Navin Gupta]

There is one problem that Searchattributes table is not fixed. If we don't find
in the table, we think it is a custom header. We could make it so that if the
custom header is not in the pref, then we should return not found. 

Comment 33

[David :Bienvenu]

yes, that's true, we should fix whatever's going on in the search attribute
stuff too. 

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Extending the filter object as suggested by David in comment 31 sounds like the
right thing to me.  

Another feature that is likely to want entries in the filter file is generic
mail filtering plugins (ie when the code that has already landed for spam stuff
gets made more generic, possibly in such a way that it can be used in regular
filters, as suggested by alecf in the spam plugin bug).

Comment 35

[Alec Flett]

Just my 2 cents - I think using mork is a fantastic idea. I've always though
having our own proprietary format (ok, so is mork in a lot of ways, but the
format allows for extensiblity) was going to bite us, and here it has :)

Mork provides the flexibility that dmose describes is needed - it could allow
filters to store arbitrary pieces of data about a given filter.

My suggestion though would be to add a small shim layer to the API to hide the
fact that we're actually using mork for the db format - or maybe add a
nsIProperties <-> mork shim so that the filter format could access metadata from
the rules store via a well defined (and frozen!) interface...

As much as people complain about mork, its really a nice little db and seems
perfect for this purpose.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

alecf: I don't get it; what does using mork buy us that can't easily be
implemented directly in the filter object?  Presumably it's not gonna help our
memory footprint any.

Comment 37

[Navin Gupta]

Comment on attachment 99168 [details] [diff] [review]
patch to fork rules.dat

marking sr=bienvenu

Comment 38

[Navin Gupta]

Attachment: patch so that we don't have to fork in future

Some of the diffs cannot be applied because it collides w/ multiple filter
actions patch. This patch handles filters it doesn't recognizes and hold the
content of such a filter in m_unparsedBuffer. We mark such a filter as
"ignoreMe" and disable it when we read them in. When we write back, we just
write this whole buffer. I have added UI checks so that we are not able to
edit/enable such a filter. Also we are not adding custom headers when we read
filters in, so that we know that this is something not recognized by this
version of mozilla and we can just ignore the whole filter. tested patch for
all cases where I have ignored filter. I also added new attributes and the
patch does ignore such a filter. david, please review, thx.

Comment 39

[Navin Gupta]

Attachment: patch so that we don't have to fork in future

The explanation remains the same. I edited part of diffs incorrectly.

Comment 40

[David :Bienvenu]

in general, it looks OK. However, the attribute "ignoreMe" is a kind of odd
name. Perhaps something like "unparseable" would make the code read a little
better and be more understandable. Or "couldNotBeParsed" - something that gives
an idea why we're treating this filter special.

Comment 41

[David :Bienvenu]

If I'm reading this correctly, whenever we see a new filter in the file (i.e., a
line that starts "name="), then we take care of the previous filter, as far as
setting the unparsed buffer, disabling, etc. What happens if the last filter in
the file is one that we can't parse? Are we handling that case?

Comment 42

[Navin Gupta]

yes, we are handling that case outside that while loop in these lines

+
+  if (m_curFilter)
+  {
+    PRBool unparseableFilter;
+    m_curFilter->GetUnparseable(&unparseableFilter);
+    if (unparseableFilter)
+    {
+      m_curFilter->SetUnparsedBuffer(m_unparsedFilterBuffer.get());
+      m_curFilter->SetEnabled(PR_FALSE);  //disable the filter because we don't
know how to apply it
+    }
+  }
+

Comment 43

[David :Bienvenu]

ok, thx. that looks ok.

Comment 44

[Navin Gupta]

Attachment: patch for not forking in future

changed attribute name "ignoreMe" to "unparseable"

Comment 45

[David :Bienvenu]

Comment on attachment 104696 [details] [diff] [review]
patch for not forking in future

thx, r=bienvenu

Comment 46

[Navin Gupta]

Cavin, Can I get r=? for last patch.

Comment 47

[Navin Gupta]

I was thinking of calling new file "filterRules.dat". 

Comment 48

[David :Bienvenu]

that would be OK, or msgFilterRules.dat.

Comment 49

[Cavin Song]

Comment on attachment 104696 [details] [diff] [review]
patch for not forking in future

r=cavin if the following are addressed:

1)
In nsresult nsMsgFilter::SaveToTextFile(), need to check argument 'aStream'
since it's referenced when m_unparseable is true.

2)
+  //nsCString		      m_originalServerPath;  //used for 4.x filters

Should we remove this line instead?

3)
+	 m_unparsedFilterBuffer = "";
Should we use m_unparsedFilterBuffer.Truncate() instead?

4)
+nsMsgSearchTerm::ParseOperator(char *inStream, nsMsgSearchOpValue *value)

Should check argument 'value' before referencing it.

Comment 50

[Navin Gupta]

both patches checked in. addressed cavin's last comment.

Comment 51

[Navin Gupta]

marking fixed

Comment 52

[esther]

verified