-
Notifications
You must be signed in to change notification settings - Fork 174
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
Resolve all conflicts for these commits #56
Conversation
Add some tests for DiagnosticsStatusNotification and FirmwareStatusNotification.
Add some tests for DiagnosticsStatusNotification and FirmwareStatusNotification.
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
============================================
- Coverage 52.71% 51.96% -0.76%
- Complexity 710 760 +50
============================================
Files 164 179 +15
Lines 2796 2998 +202
Branches 201 221 +20
============================================
+ Hits 1474 1558 +84
- Misses 1241 1355 +114
- Partials 81 85 +4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Now I just need to check with spec.
|
||
MIT License | ||
|
||
Copyright (C) 2016-2018 Thomas Volden <[email protected]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be your name and e-mail (yes I found one :P)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just check you ;)
|
||
@Override | ||
public int hashCode() { | ||
return 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 7? What argument is behind this decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as in previous situation. Just unique in compare with other classes. If you don't agree with me - I'd be happy if you suggest a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no, just curious and mostly based on lack of knowledge of the hash function. You only use prime numbers right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, only uniqueness matters. I just love prime numbers, who don't love them? If you want you may look at this http://www.baeldung.com/java-hashcode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumlin funnily the authors of the protocol haven't thought about nonces ot timestamps which makes it vulnerable to replay attacks. To the topic - I would propose to use something like UpdateFirmwareConfirmation.class.hashCode();
or UpdateFirmwareConfirmation.class.getCanonicalName().hashCode();
in such classes without fields which might be easier to maintain than sequential numbers. Equals might be simplified to return this == obj || obj instanceof UpdateFirmwareConfirmation;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- They've got MessageIDs in SOAP part for request-response mapping and permanent connection for JSON part. I think it's enough for preventing of reply attacks.
- Yeah, awesome decision, I'll rewrite it. But instanceof can't be used, look at a documentation https://docs.oracle.com/javase/specs/#instanceof . instanceof looks at parents too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumlin sorry for the off-topic - I am not sure where it's better to handle such topic. What would you advise to make JSON part secure via permanent connection? Even if we prohibit sessions' replacement, Charge Points with Wi-Fi or mobile connection still are vulnerable due to easiness of breaking connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eupakhomov I'm glad to give you some advises there https://gitter.im/ChargeTimeEU/Java-OCA-OCPP :) We're talking in this chat about all OCPP stuff, you are able to log in by Github account.
} | ||
|
||
@Test | ||
public void validate_locationIsNotSet_returnsFalse() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test doesn't seem to math the name.
I would suggest validate_returnFalse
Should there be tests for when location is set, but retrieveDate isn't and vise versa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be tests for when location is set, but retrieveDate isn't and vise versa?
@TVolden I've looked at another tests, they have no these tests. If you want I'll do that tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no biggie. Just to test validation via state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some data types that can cause some trouble. One rule which I couldn't find in the spec. Small stuff.
private String location; | ||
private Integer retries; | ||
private Calendar retrieveDate; | ||
private int retryInterval; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should change this to the nullable Integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an optional value, right. FIxed.
* | ||
*/ | ||
@XmlElement | ||
public void setRetryInterval(int retryInterval) throws PropertyConstraintException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should change to the nullable Integer
to avoid trouble with the communicator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an optional value, right. FIxed.
* | ||
* @return int, retry interval. | ||
*/ | ||
public int getRetryInterval() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should change to the nullable Integer to avoid trouble with the communicator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an optional value, right. FIxed.
if (listVersion < -1) throw new PropertyConstraintException("listVersion", listVersion, "Exceeded limit"); | ||
this.listVersion = listVersion; | ||
} | ||
private int listVersion = -2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be changed to the nullable Integer
, so it defaults to null instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a required field, it must has a value and must not be nullable.
|
||
public GetLocalListVersionConfirmation() { } | ||
|
||
public GetLocalListVersionConfirmation(int listVersion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be changed to the nullable Integer, to avoid trouble with the communicator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a required field, it must has a value and must not be nullable.
* @param reservationId Integer, id of the reservation. | ||
*/ | ||
@XmlElement | ||
public void setReservationId(Integer reservationId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have a null guard.
* @param status ReservationStatus, status of the reservation. | ||
*/ | ||
@XmlElement | ||
public void setStatus(ReservationStatus status) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have a null guard.
*/ | ||
@XmlElement | ||
public void setConnectorId(Integer connectorId) throws PropertyConstraintException { | ||
if (connectorId < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a null check also.
*/ | ||
@XmlElement | ||
public void setExpiryDate(Calendar expiryDate) { | ||
this.expiryDate = expiryDate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have a null gate.
*/ | ||
@XmlElement | ||
public void setReservationId(Integer reservationId) { | ||
this.reservationId = reservationId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have a null guard.
No description provided.