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

Feature/27616 mappings extraction #653

Open
wants to merge 123 commits into
base: master
Choose a base branch
from

Conversation

DC2-DanielKrueger
Copy link
Contributor

unit tests are broken
unit tests are compiling but broken because of the missing migration
we can load the legacy config and transform it to the new information
missing: We need to be able to write it somehow.
adaper migration almost works
it just compiles(maybe works)
@@ -309,7 +314,19 @@ public int getDepth() {
final Map<String, Object> config = objectMapper.convertValue(adapter.getConfig(), new TypeReference<>() {
});

protocolAdapterManager.updateAdapterConfig(adapterId, config);

// TODO we need to extract the mappings from the adapter object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we doing with these TODOs?
Another PR?

return ErrorResponseUtil.errorResponse(HttpStatus.NOT_FOUND_404,"Adapter not found", "The adapter named '" +
domainTag.getProtocolId() +
"' does not exist.");
return ErrorResponseUtil.errorResponse(HttpStatus.NOT_FOUND_404,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we log this?

.entity(new TagSchema(protocolId,
customConfigSchemaGenerator.generateJsonSchema(info.tagConfigurationClass())))
.build())
.orElseGet(() -> ErrorResponseUtil.errorResponse(404,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log message?

import java.util.Map;
import java.util.stream.Collectors;

@SuppressWarnings({"FieldMayBeFinal", "unused"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we donÄt use final here because of JAXB deserialization?
Might be worth a comment.


public void migrate() {
try {
// TODO check whether the migration is needed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's what you are working on right now?

if (protocolAdapterFactory == null) {
// not much we can do here. We do not have the factory to get the necessary information from
//TODO log
log.error("Fucked");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh ... that needs to go (I know I put it there ;) )

System.exit(1);
}
log.error("Could not read the configuration file {}. Exiting HiveMQ Edge.", configFile.getAbsolutePath());
log.debug("Original error message:", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this to the. error log.
This information will be vital in case we really run int osomething like that.

import java.util.Map;

public class ProtocolAdapterConfigurationServiceImpl implements ProtocolAdapterConfigurationService {

private @NotNull Map<String, Object> allConfigs = Map.of();
private @NotNull List<ProtocolAdapterEntity> allConfigs = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be thread-safe?

private final int maxQoS;

public FromEdgeMapping(
@NotNull final String tagName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotNull / final ordering

}

public @NotNull List<Map<String, Object>> tagsToMaps(final @NotNull List<? extends Tag> tags) {
return tags.stream().map(tag -> mapper.convertValue(tag, new TypeReference<Map<String, Object>>() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having map and collect each on a new line makes reading easier.

private final @NotNull String tagName;
private final @NotNull String topicFilter;
private final int mqttMaxQoS;
private final @com.hivemq.extension.sdk.api.annotations.NotNull FieldMappings fieldMappings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong NotNull used.

Copy link
Contributor

@codepitbull codepitbull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of commented out code.
Didn't really comment on it but it would be good to add comments why it is commented out.

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

Successfully merging this pull request may close these issues.

2 participants