You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by "David W. Van Couvering" <Da...@Sun.COM> on 2006/05/02 06:07:21 UTC
DRDA folks: Please review DERBY-846 patch
This patch is the first patch internationalizing strings in the network
part of the network client. I thought it would be good to get the eyes
of some of the folks working in this area on this, to make sure it makes
sense to you.
http://issues.apache.org/jira/browse/DERBY-846?page=all
Thanks,
David
Re: DRDA folks: Please review DERBY-846 patch
Posted by Andreas Korneliussen <an...@sun.com>.
David W. Van Couvering wrote:
> Hi, Andreas. Thanks for reviewing this.
>
> I was trying to give the other-side-of-pond folks a day to review it by
> posting the patch and request for review last night.
>
Well, thanks for contributing this important piece of work.
I think I missed the first request for review. I guess it only got my
full attention when you posted the deadline.
> How much time do you need?
I cannot review it more tonight - I am fine with your responses to my
comments.
I will look at it tomorrow, and let you know if I have found any issues,
regardless if you decide to check-in or not.
Andreas
>
> Let me respond to your comments below.
>
> Andreas Korneliussen wrote:
>
>> David W. Van Couvering wrote:
>>
>>> Hi, all. This patch is blocking further i18n work for me. I also
>>> don't want to have to do any merges and re-run derbyall.
>>>
>>> If you could get any comments in by noon, that would be appreciated,
>>> otherwise I will go ahead and check in and we can fix anything
>>> post-checkin.
>>>
>> This was quite a short deadline - if you want comments from the other
>> side of the world, it would be wise to extend the deadline.
>>
>> I have only looked briefly at the diff file. I think that the changes
>> in NetConnection40 and NetResultSet40 are not necessary (it appears
>> the changes only fix code indentation), so to avoid getting other
>> developers into merge conflicts, you could probably revert those changes.
>>
>> Another issue, is this error message:
>> ERROR 08004: Connection authorization failure occurred. Reason:
>> userid invalid.
>>
>> I think, for security policy reasons, the message should not reveal
>> that userid is invalid.
>
>
> Hm, I agree. That said, "invalid userid" and "invalid password" are
> both standard DRDA values for the SECCHKCD parameter. I guess I can say
> "invalid userid or password" or "invalid credentials" for both of them,
> that's a pretty common approach.
>
>>
>> Also, shouldn't it read "authentication failed", instead of
>> "authorization failure occured" ?
>
>
> Yes, I agree, I can change that.
>
>>
>> (note: the patch did not really introduce this last issue, only fixed
>> the message to have messageid as well)
>>
>>
>> -- Andreas
>>
>>> Thanks,
>>>
>>> David
>>>
>>> David W. Van Couvering wrote:
>>>
>>>> This patch is the first patch internationalizing strings in the
>>>> network part of the network client. I thought it would be good to
>>>> get the eyes of some of the folks working in this area on this, to
>>>> make sure it makes sense to you.
>>>>
>>>> http://issues.apache.org/jira/browse/DERBY-846?page=all
>>>>
>>>> Thanks,
>>>>
>>>> David
>>
>>
Re: DRDA folks: Please review DERBY-846 patch
Posted by "David W. Van Couvering" <Da...@Sun.COM>.
Hi, Andreas. Thanks for reviewing this.
I was trying to give the other-side-of-pond folks a day to review it by
posting the patch and request for review last night.
How much time do you need?
Let me respond to your comments below.
Andreas Korneliussen wrote:
> David W. Van Couvering wrote:
>> Hi, all. This patch is blocking further i18n work for me. I also
>> don't want to have to do any merges and re-run derbyall.
>>
>> If you could get any comments in by noon, that would be appreciated,
>> otherwise I will go ahead and check in and we can fix anything
>> post-checkin.
>>
> This was quite a short deadline - if you want comments from the other
> side of the world, it would be wise to extend the deadline.
>
> I have only looked briefly at the diff file. I think that the changes in
> NetConnection40 and NetResultSet40 are not necessary (it appears the
> changes only fix code indentation), so to avoid getting other developers
> into merge conflicts, you could probably revert those changes.
>
> Another issue, is this error message:
> ERROR 08004: Connection authorization failure occurred. Reason: userid
> invalid.
>
> I think, for security policy reasons, the message should not reveal that
> userid is invalid.
Hm, I agree. That said, "invalid userid" and "invalid password" are
both standard DRDA values for the SECCHKCD parameter. I guess I can say
"invalid userid or password" or "invalid credentials" for both of them,
that's a pretty common approach.
>
> Also, shouldn't it read "authentication failed", instead of
> "authorization failure occured" ?
Yes, I agree, I can change that.
>
> (note: the patch did not really introduce this last issue, only fixed
> the message to have messageid as well)
>
>
> -- Andreas
>> Thanks,
>>
>> David
>>
>> David W. Van Couvering wrote:
>>
>>> This patch is the first patch internationalizing strings in the
>>> network part of the network client. I thought it would be good to
>>> get the eyes of some of the folks working in this area on this, to
>>> make sure it makes sense to you.
>>>
>>> http://issues.apache.org/jira/browse/DERBY-846?page=all
>>>
>>> Thanks,
>>>
>>> David
>
Re: DRDA folks: Please review DERBY-846 patch
Posted by "David W. Van Couvering" <Da...@Sun.COM>.
I forgot to respond to one of your comments, see below...
Andreas Korneliussen wrote:
>
> I have only looked briefly at the diff file. I think that the changes in
> NetConnection40 and NetResultSet40 are not necessary (it appears the
> changes only fix code indentation), so to avoid getting other developers
> into merge conflicts, you could probably revert those changes.
The reason I did this was very selfish. I have a script that runs
through all the code and tries to find as many places where a
client-side SqlException is created. I then use this script to generate
a test that makes sure that these exceptions are being created
correctly: e.g. they use a message id that has a matching message, and
that they pass the right number of arguments. This has turned out to be
*very* valuable.
The problem is, for reasons I won't get into, the changes I made to
NetConnection40 and NetResultSet40 were to prevent the script from
breaking. I could have modified the script, but it was a lot of work,
and this was an easy change.
I hope the merge impact isn't too heavy, this little change saved me a
lot of time.
If you think it's going to be a big issue, let me know.
David
Re: DRDA folks: Please review DERBY-846 patch
Posted by Andreas Korneliussen <an...@sun.com>.
David W. Van Couvering wrote:
> Hi, all. This patch is blocking further i18n work for me. I also don't
> want to have to do any merges and re-run derbyall.
>
> If you could get any comments in by noon, that would be appreciated,
> otherwise I will go ahead and check in and we can fix anything
> post-checkin.
>
This was quite a short deadline - if you want comments from the other
side of the world, it would be wise to extend the deadline.
I have only looked briefly at the diff file. I think that the changes in
NetConnection40 and NetResultSet40 are not necessary (it appears the
changes only fix code indentation), so to avoid getting other developers
into merge conflicts, you could probably revert those changes.
Another issue, is this error message:
ERROR 08004: Connection authorization failure occurred. Reason: userid
invalid.
I think, for security policy reasons, the message should not reveal that
userid is invalid.
Also, shouldn't it read "authentication failed", instead of
"authorization failure occured" ?
(note: the patch did not really introduce this last issue, only fixed
the message to have messageid as well)
-- Andreas
> Thanks,
>
> David
>
> David W. Van Couvering wrote:
>
>> This patch is the first patch internationalizing strings in the
>> network part of the network client. I thought it would be good to get
>> the eyes of some of the folks working in this area on this, to make
>> sure it makes sense to you.
>>
>> http://issues.apache.org/jira/browse/DERBY-846?page=all
>>
>> Thanks,
>>
>> David
Re: DRDA folks: Please review DERBY-846 patch
Posted by "David W. Van Couvering" <Da...@Sun.COM>.
Hi, all. This patch is blocking further i18n work for me. I also don't
want to have to do any merges and re-run derbyall.
If you could get any comments in by noon, that would be appreciated,
otherwise I will go ahead and check in and we can fix anything post-checkin.
Thanks,
David
David W. Van Couvering wrote:
> This patch is the first patch internationalizing strings in the network
> part of the network client. I thought it would be good to get the eyes
> of some of the folks working in this area on this, to make sure it makes
> sense to you.
>
> http://issues.apache.org/jira/browse/DERBY-846?page=all
>
> Thanks,
>
> David