-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix:Output to console the key and value that was inserted by a I… #3377
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,59 @@ | ||||||
/* | ||||||
* Copyright 2018 Confluent Inc. | ||||||
* | ||||||
* Licensed under the Confluent Community License (the "License"); you may not use | ||||||
* this file except in compliance with the License. You may obtain a copy of the | ||||||
* License at | ||||||
* | ||||||
* http://www.confluent.io/confluent-community-license | ||||||
* | ||||||
* Unless required by applicable law or agreed to in writing, software | ||||||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||||||
* WARRANTIES OF ANY KIND, either express or implied. See the License for the | ||||||
* specific language governing permissions and limitations under the License. | ||||||
*/ | ||||||
|
||||||
package io.confluent.ksql.rest.entity; | ||||||
|
||||||
import com.fasterxml.jackson.annotation.JsonIgnoreProperties; | ||||||
import com.fasterxml.jackson.annotation.JsonProperty; | ||||||
import java.util.List; | ||||||
import java.util.Objects; | ||||||
import java.util.Optional; | ||||||
|
||||||
@JsonIgnoreProperties(ignoreUnknown = true) | ||||||
public class MessageEntity extends KsqlEntity { | ||||||
|
||||||
private final Optional<String> message; | ||||||
|
||||||
public MessageEntity( | ||||||
@JsonProperty("statementText") final String statementText, | ||||||
@JsonProperty("warnings") final List<KsqlWarning> warnings, | ||||||
@JsonProperty("message") final Optional<String> message) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would the message not be provided? Isn't the whole point of this entity type to return a message?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea is MessageEntity could be used by a different endpoint which produces an optional message. E.g. maybe we decide to only print the insert results if the key is null. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get it... this is a |
||||||
super(statementText, warnings); | ||||||
this.message = Objects.requireNonNull(message); | ||||||
} | ||||||
|
||||||
public Optional<String> getMessage() { | ||||||
return message; | ||||||
} | ||||||
|
||||||
|
||||||
@Override | ||||||
public boolean equals(final Object o) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. incorrect hashCode and equals, as they're not taking into account the parent class's fields. I'd suggest adding a unit test using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the other KSqlEntity subclasses, most (all?) seem to suffer from this problem. So we should probably fix them all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually wonder why hashCode() and equals() are overridden for these classes in the first place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If they are needed, then yes, but that's a different PR ;) . What's more important is ensuring any new code is correct.
Totally. Often its only for tests to ensure you can assert on output. I'm not normally a fan of having production code just for tests, but I can live with this. However, is it's in the production code base it should be correct! So we should either fix this or remove it. |
||||||
if (this == o) { | ||||||
return true; | ||||||
} | ||||||
if (o == null || getClass() != o.getClass()) { | ||||||
return false; | ||||||
} | ||||||
final MessageEntity that = (MessageEntity) o; | ||||||
return message.equals(that.message); | ||||||
} | ||||||
|
||||||
@Override | ||||||
public int hashCode() { | ||||||
return Objects.hash(message); | ||||||
} | ||||||
} | ||||||
|
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.
Don't accept
warnings
if you're only passingnull
/ empty list. Just remove the constructor param and pass an empty list to the super class.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 don't have warnings for insert into, but I left it in there as MessageEntity could be used by a different endpoint which does produce warnings.
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 easy enough for someone to put the warnings parameter back if they need it. The point is, at the moment its not needed, and I'm not even sure it a warning would get output in the CLI even if one was supplied.
Better to keep the API to want we require now. We may never need to pass a warning via this class...