refactor(api): Move EventListener and dependent classes to echo-api, remove previous RestEventParser #790
Conversation
…remove previous RestEventParser
echo-api/src/main/java/com/netflix/spinnaker/echo/api/events/Event.java
Outdated
Show resolved
Hide resolved
jonsie
changed the title
| import com.fasterxml.jackson.annotation.JsonInclude; | ||
| import java.util.Map; | ||
|
|
||
| public abstract class EventMixin { |
|
|
||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public Map payload; | ||
| public Map<String, Object> payload; | ||
|
|
||
| public String eventId = UUID.randomUUID().toString(); | ||
| } |
There was a problem hiding this comment.
Is there value in content and payload being typed? I'd really like to avoid bringing M<S, O> into the -api modules if possible. Perhaps for Echo, we can't really do anything about it until the other services sending events in are well-defined?
There was a problem hiding this comment.
I think I'll merge as is and then explore how content and payload could be typed. There's a lot to unwind WRT how Event and TriggerEvent (and the various TriggerEvent types) deserialize the content field and I don't feel comfortable making any changes there yet.
echo-api/src/main/java/com/netflix/spinnaker/echo/api/events/EventListener.java
Show resolved
Hide resolved
| eventMap = | ||
| restEventParser | ||
| .get() | ||
| .parseEvent(mapper.convertValue(event, RestEventParser.Event.class)); |
There was a problem hiding this comment.
I see this code being removed here, but I don't recall seeing it put somewhere else. It's concerning to me that this didn't break any tests, either?
There was a problem hiding this comment.
This is the integration for the first extension point I added to echo (RestEventParser), but I think there's more value in pulling it out and redesigning how we build the echo-api. It's not being used at all right now.
| private Map attributes; | ||
| private HttpHeaders requestHeaders = new HttpHeaders(); | ||
| private Map<String, String> attributes; | ||
| private Map<String, List<String>> requestHeaders = new HashMap<>(); |
There was a problem hiding this comment.
@jonsie It looks like this broke code that relied on the assumption of headers being converted to lowercase by HttpHeaders, as discussed in #840. I think it might be best to revert this line instead of fixing each specific downstream issue that comes up as a result, unless there was a good reason to convert this particular property to a Map.
There was a problem hiding this comment.
Ah sorry, I didn't catch this. Unfortunately, we can't pull in the Spring dependency into echo-api as this module needs to be dependency free (basically, just Java), which is why I removed HttpHeaders.
There was a problem hiding this comment.
This might do the trick:
private Map<String, List<String>> requestHeaders = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
Todo:
@JsonInclude(NON_NULL)from echo-api). Likely will be in the moduleecho-api-jackson.ObjectMapperconfiguration (so custom serializers/deserializers are always configured)I also took the opportunity to rename
EchoEventListenerto justEventListener.