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

Fields should be marked transient only if class is Serializable #675

Closed
krzyk opened this issue Feb 2, 2016 · 31 comments
Closed

Fields should be marked transient only if class is Serializable #675

krzyk opened this issue Feb 2, 2016 · 31 comments
Assignees
Labels

Comments

@krzyk
Copy link
Collaborator

krzyk commented Feb 2, 2016

Currently we have a PMD rule BeanMembersShouldSerialize with rationale:

If a class is a bean, or is referenced by a bean directly or indirectly it needs to be serializable. Member variables need to be marked as transient, static, or have accessor methods in the class. Marking variables as transient is the safest and easiest modification. Accessor methods should follow the Java naming conventions, i.e. for a variable named foo, getFoo() and setFoo() accessor methods should be provided.

Most of the time we don't follow the rules for Java Beans (see getters/setters) so I think that the rule requiring fields to be transient should be loose up to only Serializable classes - and there is already such rule enabled in Findbugs (SE_BAD_FIELD):

This Serializable class defines a non-primitive instance field which is neither transient, Serializable, or java.lang.Object, and does not appear to implement the Externalizable interface or the readObject() and writeObject() methods. Objects of this class will not be deserialized correctly if a non-Serializable object is stored in this field.

We should remove rule BeanMembersShouldSerialize and create a test that ensures we can create classes without transient fields.

@krzyk
Copy link
Collaborator Author

krzyk commented Feb 2, 2016

@yegor256 What do you think about the above?

@yegor256
Copy link
Owner

yegor256 commented Feb 3, 2016

@krzyk well... what's wrong with transient? it is an extra protection against accidental serialization, as far as I understand.

@krzyk
Copy link
Collaborator Author

krzyk commented Feb 3, 2016

@yegor256 I haven't thought of it as a protection, more like wasted keystrokes :)
If class is not Serializable then how could it be serialized by accident? One would have to do it on purpose (writeObject) in class that uses it.
And lomboks @EqualsAndHashCode requires naming each transient field for the generated methods (and this annotation is easily forgotten when refactoring the class).

@yegor256
Copy link
Owner

yegor256 commented Feb 3, 2016

@krzyk OK, I'm convinced, let's remove this check

@krzyk
Copy link
Collaborator Author

krzyk commented Feb 4, 2016

@davvd valid bug

@davvd
Copy link

davvd commented Feb 4, 2016

@davvd valid bug

@krzyk I just added bug tag here

@krzyk
Copy link
Collaborator Author

krzyk commented Feb 8, 2016

@davvd this is urgent

vkuchyn added a commit to vkuchyn/qulice that referenced this issue Feb 8, 2016
@davvd
Copy link

davvd commented Feb 9, 2016

@davvd this is urgent

@krzyk OK, I put "urgent" label here

@davvd davvd added the urgent label Feb 9, 2016
@davvd
Copy link

davvd commented Feb 9, 2016

@vkuchyn please pick this up, and keep in mind these instructions. Any technical questions - ask right here; The budget here is 30 mins, which is exactly how much time will be paid for, when the task is completed

@davvd
Copy link

davvd commented Feb 10, 2016

@krzyk many thanks, 30 mins added to your acc for reporting this bug, pmt ID 77111221

vkuchyn added a commit to vkuchyn/qulice that referenced this issue Feb 10, 2016
vkuchyn added a commit to vkuchyn/qulice that referenced this issue Feb 10, 2016
vkuchyn added a commit to vkuchyn/qulice that referenced this issue Feb 10, 2016
vkuchyn added a commit to vkuchyn/qulice that referenced this issue Feb 10, 2016
@krzyk
Copy link
Collaborator Author

krzyk commented Feb 10, 2016

@vkuchyn it is a good practice to add reference to the PR, even before it was merged

@krzyk krzyk closed this as completed Feb 10, 2016
@davvd
Copy link

davvd commented Feb 12, 2016

@vkuchyn I'm not sure who actually completed this task, since there were no messages from you above (correct me if I'm wrong)

@krzyk
Copy link
Collaborator Author

krzyk commented Feb 12, 2016

@vkuchyn you have to always inform David about PR in the issue you are solving, and after it was merged ask issue creator to close IT (unless creator notices it earlier, like in this case). After you add the reference, please ask davdd for payment.

@vkuchyn
Copy link
Contributor

vkuchyn commented Feb 12, 2016

@davvd Issue was fixed, see #698. Please, proceed payments

@davvd
Copy link

davvd commented Feb 15, 2016

@davvd Issue was fixed, see #698. Please, proceed payments

@vkuchyn thank you!

@vkuchyn
Copy link
Contributor

vkuchyn commented Feb 19, 2016

@davvd issue was fixed 7 days ago, but still no payments. Please, recheck status of it.

@vkuchyn
Copy link
Contributor

vkuchyn commented Feb 25, 2016

@davvd More than 10 days passed and still no payment. Please - fix it.
@krzyk 10 days passed - no payment, no reaction on my comment above. What is the best way to resolve this situation according your working policy? Thanks.

@krzyk
Copy link
Collaborator Author

krzyk commented Feb 26, 2016

@vkuchyn you can ask @yegor256 for help here

@vkuchyn
Copy link
Contributor

vkuchyn commented Feb 26, 2016

@krzyk Thanks
@yegor256 please, help to resolve the situation - read comments above.

@yegor256
Copy link
Owner

@davvd pls add extra 30 mins to @vkuchyn

@davvd
Copy link

davvd commented Feb 29, 2016

@davvd issue was fixed 7 days ago, but still no payments. Please, recheck status of it.

@vkuchyn @yegor256 please help us here

@davvd
Copy link

davvd commented Feb 29, 2016

@davvd pls add extra 30 mins to @vkuchyn

@yegor256 I've sent 30 mins extra to @vkuchyn in transaction ID "78701880"

@vkuchyn
Copy link
Contributor

vkuchyn commented Mar 1, 2016

@davvd Thank you for payment. What about raiting? This task tool 3 days according to github history. But it is not mentioned in my netbout rating. Please check it and fix.

@krzyk
Copy link
Collaborator Author

krzyk commented Mar 2, 2016

@davvd this is not urgent

@davvd davvd removed the urgent label Mar 2, 2016
@davvd
Copy link

davvd commented Mar 2, 2016

@davvd this is not urgent

@krzyk right, I removed "urgent" tag

@vkuchyn
Copy link
Contributor

vkuchyn commented Mar 10, 2016

@yegor256 again need your help. Please, see comment above

@yegor256
Copy link
Owner

@vkuchyn
Copy link
Contributor

vkuchyn commented Mar 11, 2016

@yegor256 please, see http://at.teamed.io/policy.html#14 I didn't received any points, but got payment - what I am talking about

@yegor256
Copy link
Owner

@vkuchyn you still don't have rating points in your account in Netbout?

@vkuchyn
Copy link
Contributor

vkuchyn commented Mar 15, 2016

@yegor256 still don't have this ticket in my rating. You can take a look by the reference http://www.netbout.com/b/36353?open=rating if you have access

@yegor256
Copy link
Owner

@vkuchyn that's because the payment was done not in a normal mode. won't happen in the future, I hope. we have to move on here, can't do anything with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants