Tagging is code-complete

Hi Guys,

I consider tagging to be code-complete, but definitely not
bug-free. Now
I'll start working with DB2 to make sure we are compatible
in the SQL
queries/structure and keep on testing. Please review/test it
to make
sure I'm not missing anything really important.

Thanks.

-Elias

The WeblogEntry unit test is reporting a number of failures
for me.

-- Allen


Elias Torres wrote:
> Hi Guys,
> 
> I consider tagging to be code-complete, but definitely
not bug-free. Now
> I'll start working with DB2 to make sure we are
compatible in the SQL
> queries/structure and keep on testing. Please
review/test it to make
> sure I'm not missing anything really important.
> 
> Thanks.
> 
> -Elias

FYI,

I chatted with Allen and these errors were already fixed.
Everyone
please make sure you update to the latest trunk.

-Elias

Allen Gilliland wrote:
> The WeblogEntry unit test is reporting a number of
failures for me.
> 
> -- Allen
> 
> 
> Elias Torres wrote:
>> Hi Guys,
>>
>> I consider tagging to be code-complete, but
definitely not bug-free. Now
>> I'll start working with DB2 to make sure we are
compatible in the SQL
>> queries/structure and keep on testing. Please
review/test it to make
>> sure I'm not missing anything really important.
>>
>> Thanks.
>>
>> -Elias
> 

Elias,

I am still playing around with the latest tagging code in
the trunk, but 
so far everything looks great.  I have a few more small
items that I 
think are worth doing, and one or two optional ones we can
think about ...

1. I think we should allow for configurable values for a
couple things.

   - # of allowed tag intersections.  you hard coded 3, but
i think this 
should be configurable.  this is enforced in
WeblogXXXRequest.
   - maximum length value returned from getPopularTags(). 
right now you 
just accept the value passed in, but i think we should do a
simple check 
to make sure the value passed in doesn't exceed some
configured max. 
this is enforced in WebsiteData and SiteModel
getPopularTags() methods.

2. You don't need a getTags() method in SiteModel, but it
should be in 
FeedModel.  Since the SiteModel is stateless there is no
need for that 
method, it's already available in the PageModel and you
should add it to 
the FeedModel as well.

Those are the only ones I have so far which I think should
be done. 
There are a couple more optional ones which may be worth
doing as well, 
but aren't necessarily tied to tagging ...

3. Enforce strict urls.  This is more of an overall approach
type 
question to url handling, but right now we allow for
opportunities where 
unsupported data is put into a url and instead of rejecting
the url we 
simply ignore the parts we don't support.  This may not be
the best idea 
depending on how you look at it.  Since technically we don't
want people 
to try and do /weblog/tags/foo+bar?cat=MyCat then it makes
some sense to 
reject those urls as 404 or possibly a 400 Bad Request. 
Obviously this 
isn't really a problem, it's more of a question of best
practice.

4. Optionally enforce a max entries per page setting.  We
had briefly 
talked about this during the 3.0 release, but it didn't end
up getting 
done.  In any case, I think this is a good thing to be a
site-wide 
configurable setting so that the admin can define the
maximum number of 
entries to show on a page, and the default Roller value can
be 
"unlimited".  On large scale sites like
blogs.sun.com people make some 
rather silly decisions, like setting the entry display count
to "9999", 
and so enforcing a max value seems quite reasonable to me. 
I had 
planned to do this for 3.0, but it slipped by me, so I can
take this as 
an item for 3.1.

-- Allen


Elias Torres wrote:
> Hi Guys,
> 
> I consider tagging to be code-complete, but definitely
not bug-free. Now
> I'll start working with DB2 to make sure we are
compatible in the SQL
> queries/structure and keep on testing. Please
review/test it to make
> sure I'm not missing anything really important.
> 
> Thanks.
> 
> -Elias

Allen Gilliland wrote:
> Elias,
> 
> I am still playing around with the latest tagging code
in the trunk, but
> so far everything looks great.  I have a few more small
items that I
> think are worth doing, and one or two optional ones we
can think about ...
> 
> 1. I think we should allow for configurable values for
a couple things.
> 
>   - # of allowed tag intersections.  you hard coded 3,
but i think this
> should be configurable.  this is enforced in
WeblogXXXRequest.
>   - maximum length value returned from
getPopularTags().  right now you
> just accept the value passed in, but i think we should
do a simple check
> to make sure the value passed in doesn't exceed some
configured max.
> this is enforced in WebsiteData and SiteModel
getPopularTags() methods.
> 
> 2. You don't need a getTags() method in SiteModel, but
it should be in
> FeedModel.  Since the SiteModel is stateless there is
no need for that
> method, it's already available in the PageModel and you
should add it to
> the FeedModel as well.
> 
> Those are the only ones I have so far which I think
should be done.
> There are a couple more optional ones which may be
worth doing as well,
> but aren't necessarily tied to tagging ...

I will do those.

> 
> 3. Enforce strict urls.  This is more of an overall
approach type
> question to url handling, but right now we allow for
opportunities where
> unsupported data is put into a url and instead of
rejecting the url we
> simply ignore the parts we don't support.  This may not
be the best idea
> depending on how you look at it.  Since technically we
don't want people
> to try and do /weblog/tags/foo+bar?cat=MyCat then it
makes some sense to
> reject those urls as 404 or possibly a 400 Bad Request.
 Obviously this
> isn't really a problem, it's more of a question of best
practice.

Correct. I believe that if it's a bad parameter is a bad
request but if
it's in the path, then 404. I totally agree we shouldn't be
ignoring
stuff and should be reporting it accordingly.

> 
> 4. Optionally enforce a max entries per page setting. 
We had briefly
> talked about this during the 3.0 release, but it didn't
end up getting
> done.  In any case, I think this is a good thing to be
a site-wide
> configurable setting so that the admin can define the
maximum number of
> entries to show on a page, and the default Roller value
can be
> "unlimited".  On large scale sites like
blogs.sun.com people make some
> rather silly decisions, like setting the entry display
count to "9999",
> and so enforcing a max value seems quite reasonable to
me.  I had
> planned to do this for 3.0, but it slipped by me, so I
can take this as
> an item for 3.1.

No problem. Is it a small change?

> 
> -- Allen
> 
> 
> Elias Torres wrote:
>> Hi Guys,
>>
>> I consider tagging to be code-complete, but
definitely not bug-free. Now
>> I'll start working with DB2 to make sure we are
compatible in the SQL
>> queries/structure and keep on testing. Please
review/test it to make
>> sure I'm not missing anything really important.
>>
>> Thanks.
>>
>> -Elias
> 

Elias Torres wrote:
> 
> Allen Gilliland wrote:
>> Elias,
>>
>> I am still playing around with the latest tagging
code in the trunk, but
>> so far everything looks great.  I have a few more
small items that I
>> think are worth doing, and one or two optional ones
we can think about ...
>>
>> 1. I think we should allow for configurable values
for a couple things.
>>
>>   - # of allowed tag intersections.  you hard coded
3, but i think this
>> should be configurable.  this is enforced in
WeblogXXXRequest.
>>   - maximum length value returned from
getPopularTags().  right now you
>> just accept the value passed in, but i think we
should do a simple check
>> to make sure the value passed in doesn't exceed
some configured max.
>> this is enforced in WebsiteData and SiteModel
getPopularTags() methods.
>>
>> 2. You don't need a getTags() method in SiteModel,
but it should be in
>> FeedModel.  Since the SiteModel is stateless there
is no need for that
>> method, it's already available in the PageModel and
you should add it to
>> the FeedModel as well.
>>
>> Those are the only ones I have so far which I think
should be done.
>> There are a couple more optional ones which may be
worth doing as well,
>> but aren't necessarily tied to tagging ...
> 
> I will do those.
> 
>> 3. Enforce strict urls.  This is more of an overall
approach type
>> question to url handling, but right now we allow
for opportunities where
>> unsupported data is put into a url and instead of
rejecting the url we
>> simply ignore the parts we don't support.  This may
not be the best idea
>> depending on how you look at it.  Since technically
we don't want people
>> to try and do /weblog/tags/foo+bar?cat=MyCat then
it makes some sense to
>> reject those urls as 404 or possibly a 400 Bad
Request.  Obviously this
>> isn't really a problem, it's more of a question of
best practice.
> 
> Correct. I believe that if it's a bad parameter is a
bad request but if
> it's in the path, then 404. I totally agree we
shouldn't be ignoring
> stuff and should be reporting it accordingly.
> 
>> 4. Optionally enforce a max entries per page
setting.  We had briefly
>> talked about this during the 3.0 release, but it
didn't end up getting
>> done.  In any case, I think this is a good thing to
be a site-wide
>> configurable setting so that the admin can define
the maximum number of
>> entries to show on a page, and the default Roller
value can be
>> "unlimited".  On large scale sites like
blogs.sun.com people make some
>> rather silly decisions, like setting the entry
display count to "9999",
>> and so enforcing a max value seems quite reasonable
to me.  I had
>> planned to do this for 3.0, but it slipped by me,
so I can take this as
>> an item for 3.1.
> 
> No problem. Is it a small change?

yeah.  it's basically adding the property to the config and
then 
enforcing it in a couple of the pagers.  i will do it today.

-- Allen


> 
>> -- Allen
>>
>>
>> Elias Torres wrote:
>>> Hi Guys,
>>>
>>> I consider tagging to be code-complete, but
definitely not bug-free. Now
>>> I'll start working with DB2 to make sure we are
compatible in the SQL
>>> queries/structure and keep on testing. Please
review/test it to make
>>> sure I'm not missing anything really important.
>>>
>>> Thanks.
>>>
>>> -Elias

Allen Gilliland wrote:
>>> 4. Optionally enforce a max entries per page
setting.  We had briefly
>>> talked about this during the 3.0 release, but
it didn't end up getting
>>> done.  In any case, I think this is a good
thing to be a site-wide
>>> configurable setting so that the admin can
define the maximum number of
>>> entries to show on a page, and the default
Roller value can be
>>> "unlimited".  On large scale sites
like blogs.sun.com people make some
>>> rather silly decisions, like setting the entry
display count to "9999",
>>> and so enforcing a max value seems quite
reasonable to me.  I had
>>> planned to do this for 3.0, but it slipped by
me, so I can take this as
>>> an item for 3.1.
>>
>> No problem. Is it a small change?
> 
> yeah.  it's basically adding the property to the config
and then 
> enforcing it in a couple of the pagers.  i will do it
today.

I just checked this code in a couple minutes ago.

-- Allen

Allen Gilliland wrote:
> Elias,
> 
> I am still playing around with the latest tagging code
in the trunk, but
> so far everything looks great.  I have a few more small
items that I
> think are worth doing, and one or two optional ones we
can think about ...
> 
> 1. I think we should allow for configurable values for
a couple things.
> 
>   - # of allowed tag intersections.  you hard coded 3,
but i think this
> should be configurable.  this is enforced in
WeblogXXXRequest.
>   - maximum length value returned from
getPopularTags().  right now you
> just accept the value passed in, but i think we should
do a simple check
> to make sure the value passed in doesn't exceed some
configured max.
> this is enforced in WebsiteData and SiteModel
getPopularTags() methods.
> 
> 2. You don't need a getTags() method in SiteModel, but
it should be in
> FeedModel.  Since the SiteModel is stateless there is
no need for that
> method, it's already available in the PageModel and you
should add it to
> the FeedModel as well.
> 
> Those are the only ones I have so far which I think
should be done.
> There are a couple more optional ones which may be
worth doing as well,
> but aren't necessarily tied to tagging ...

Done.

I have a couple more very small things ...

1. I would prefer it if the new tables are prefixed with
"roller_". 
Obviously this has no functional benefit, but all things
being equal I 
think it's better if all of the roller tables are identified
as such. 
This has no downsides and definitely makes it easier on
users who happen 
to be mixing tables from many apps in the same database, so
I see no 
reason not to do it.

2. We don't need the TagManager interface anymore right?  So
it can be 
deleted?

-- Allen


Elias Torres wrote:
> 
> Allen Gilliland wrote:
>> Elias,
>>
>> I am still playing around with the latest tagging
code in the trunk, but
>> so far everything looks great.  I have a few more
small items that I
>> think are worth doing, and one or two optional ones
we can think about ...
>>
>> 1. I think we should allow for configurable values
for a couple things.
>>
>>   - # of allowed tag intersections.  you hard coded
3, but i think this
>> should be configurable.  this is enforced in
WeblogXXXRequest.
>>   - maximum length value returned from
getPopularTags().  right now you
>> just accept the value passed in, but i think we
should do a simple check
>> to make sure the value passed in doesn't exceed
some configured max.
>> this is enforced in WebsiteData and SiteModel
getPopularTags() methods.
>>
>> 2. You don't need a getTags() method in SiteModel,
but it should be in
>> FeedModel.  Since the SiteModel is stateless there
is no need for that
>> method, it's already available in the PageModel and
you should add it to
>> the FeedModel as well.
>>
>> Those are the only ones I have so far which I think
should be done.
>> There are a couple more optional ones which may be
worth doing as well,
>> but aren't necessarily tied to tagging ...
> 
> Done.

Allen Gilliland wrote:
> I have a couple more very small things ...
> 
> 1. I would prefer it if the new tables are prefixed
with "roller_".
> Obviously this has no functional benefit, but all
things being equal I
> think it's better if all of the roller tables are
identified as such.
> This has no downsides and definitely makes it easier on
users who happen
> to be mixing tables from many apps in the same
database, so I see no
> reason not to do it.

done.

> 
> 2. We don't need the TagManager interface anymore
right?  So it can be
> deleted?

done.

> 
> -- Allen
> 
> 
> Elias Torres wrote:
>>
>> Allen Gilliland wrote:
>>> Elias,
>>>
>>> I am still playing around with the latest
tagging code in the trunk, but
>>> so far everything looks great.  I have a few
more small items that I
>>> think are worth doing, and one or two optional
ones we can think
>>> about ...
>>>
>>> 1. I think we should allow for configurable
values for a couple things.
>>>
>>>   - # of allowed tag intersections.  you hard
coded 3, but i think this
>>> should be configurable.  this is enforced in
WeblogXXXRequest.
>>>   - maximum length value returned from
getPopularTags().  right now you
>>> just accept the value passed in, but i think we
should do a simple check
>>> to make sure the value passed in doesn't exceed
some configured max.
>>> this is enforced in WebsiteData and SiteModel
getPopularTags() methods.
>>>
>>> 2. You don't need a getTags() method in
SiteModel, but it should be in
>>> FeedModel.  Since the SiteModel is stateless
there is no need for that
>>> method, it's already available in the PageModel
and you should add it to
>>> the FeedModel as well.
>>>
>>> Those are the only ones I have so far which I
think should be done.
>>> There are a couple more optional ones which may
be worth doing as well,
>>> but aren't necessarily tied to tagging ...
>>
>> Done.
>