[
https://issues.apache.org/jira/browse/QPID-7336?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15432731#comment-15432731
]
Alex Rudyy edited comment on QPID-7336 at 8/23/16 12:59 PM:
------------------------------------------------------------
Lorenz,
Here are my review comments
* UserPreferencesImpl
** The committed changes shifted responsibility to set updateDate, createDate
and id fields from PreferenceFactory into UserPreferencesImpl. I think it is a
step in the right direction.
** checkForIdUniqueness - method name is misleading . I would use name like
"ensureSameTypeAndOwnerForExistingPreference" as it describes the method
purpose better.
** Additionally the validation performed in checkForIdUniqueness overlaps with
checkForConflictWithExisting.
I am wondering whether we really need this method. Would it be better to call
checkForConflictWithExisting instead and remove this method.
The duplication of the name would be checked as well (if name check is not
desired, perhaps we can introduce a flag to skip name validation)
** Methods principalsEqual and principalsContain can be easily inlined
* PreferencesRecoverer
** I think that validation of id, owner, createDate/updateDate fields should
be performed either in PreferenceFactory#recover or PrefernceRecoverer. Such
validation is absent at the moment, as it was removed from both
PreferenceFactory and PrefernceImp. As result, the removal of
id/owner/updateDate/createDate in store will endup in creation of preference
with null for corresponding fields on recovery. Before such records were added
into corrupted records, whilst with current changes records without id and/or
owner, etc will be considered as valid records. Accessing corrupted preferences
via REST results in 500 caused by NPE.
* PrefernceImp
** With current implementation I see a possibility for NPE whilst accessing
createData field, for example,
when preferences are edited manually and createDate field is removed and
recovery PrefernceImp would be created with null createDate.
Calling getter getCreateDate() will result in NPE.
** The same is for getUpdateDate()
* PreferenceFactory
** With the current implementation methods create and recover have been
replaced with generic fromAttributes. Such change moves the responsibility to
validate the attributes to the caller of PreferenceFactory#fromAttributes and
UserPreferences methods. IMHO, i would restore recover method and add
validation of createDate, updateDate, id, name, type and owner there.
** Additionally I find the method name "fromAttributes" untipical for
factories. Usually create factory methods called "create". I think renaming
"fromAttributes" into "create" would better describe the method purpose.
* PreferencesTest
** testOverrideContradictingOwner - the test was changed and not testing
anymore what it used supposed to test. I am not sure that it is testing
"overiding of contradicting owner" either. Is it a correct test?
was (Author: alex.rufous):
Lorenz,
Here are my review comments
* UserPreferencesImpl
** The committed changes shifted responsibility to set updateDate, createDate
and id field from PreferenceFactory into UserPreferencesImpl. I think it is a
step in the right direction.
** checkForIdUniqueness - method name is misleading . I would use name like
"ensureSameTypeAndOwnerForExistingPreference" as it describes the method
purpose better.
** Additionally the validation performed in checkForIdUniqueness overlaps with
checkForConflictWithExisting.
I am wondering whether we really need this method. Would it be better to call
checkForConflictWithExisting instead and remove this method.
The duplication of the name would be checked as well (if name check is not
desired, perhaps we can intriduce a flag to skip name validation)
** Methods principalsEqual and principalsContain can be easilly inlined
* PreferencesRecoverer
** I think that validation of id, owner, createDate/updateDate fields should
be performed either in PreferenceFactory#recover or PrefernceRecoverer. Such
validation is absent at the moment, as it was removed from both
PreferenceFactory and PrefernceImp. As result, the removal of
id/owner/updateDate/createDate in store will endup in creation of preference
with null for corresponding fields on recovery. Before such records were added
into corrupted records, whilst with current changes records without id and/or
owner, etc will be considered as valid records. Accessing corrupted preferences
via REST results in 500 caused by NPE.
* PrefernceImp
** With current implementation I see a possibility for NPE whilst accessing
createData field, for example,
when preferences are edited manually and createDate field is removed and
recovery PrefernceImp would be created with null createDate.
Calling getter getCreateDate() will result in NPE.
** The same is for getUpdateDate()
* PreferenceFactory
** With the current implementation methods create and recover have been
replaced with generic fromAttributes. Such change moves the responsibility to
validate the attributes to the caller of PreferenceFactory#fromAttributes and
UserPreferences methods. IMHO, i would restore recover method and add
validation of createDate, updateDate, id, name, type and owner there.
** Additionally I find the method name "fromAttributes" untipical for
factories. Usually create factory methods called "create". I think renaming
"fromAttributes" into "create" would better describe the method purpose.
* PreferencesTest
** testOverrideContradictingOwner - the test was changed and not testing
anymore what it used supposed to test. I am not sure that it is testing
"overiding of contradicting owner" either. Is it a correct test?
> [Java Broker] Add createdDate to Preference object
> --------------------------------------------------
>
> Key: QPID-7336
> URL: https://issues.apache.org/jira/browse/QPID-7336
> Project: Qpid
> Issue Type: Improvement
> Components: Java Broker
> Reporter: Lorenz Quack
> Assignee: Lorenz Quack
> Fix For: qpid-java-6.1
>
>
> For auditing purpose we want a field createdDate on Preference objects.
> One difficulty is that we would like Preferences to be immutable.
> This might prove problematic in the case where we update a preference.
> Special attention should be paid to this case.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
