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.

Sign up for GitHub

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

Jump to bottom

Add validation to Registrar. Add RegistrarTest to exercise it. #361

Merged
merged 18 commits into from
Jun 23, 2022
Merged

Add validation to Registrar. Add RegistrarTest to exercise it. #361

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4b3824a
Add dryrun option -n
johnrandolph Jun 2, 2022
e0c6de0
Oops, shift by the option
johnrandolph Jun 2, 2022
a6b72b2
Remove dbg
johnrandolph Jun 2, 2022
bf78213
Merge remote-tracking branch 'faucet/master'
johnrandolph Jun 2, 2022
5e9de32
Merge remote-tracking branch 'faucet/master'
johnrandolph Jun 3, 2022
00ac587
Merge remote-tracking branch 'faucet/master'
johnrandolph Jun 7, 2022
b60ecdb
First pass at RegistrarTest and slight changes to Registrar.
johnrandolph Jun 9, 2022
2ad6e87
Add validation tests
johnrandolph Jun 14, 2022
d159768
Test work
johnrandolph Jun 21, 2022
6793eff
Test improvements
johnrandolph Jun 22, 2022
0f662bb
Merge branch 'master' into master
johnrandolph Jun 22, 2022
167e8de
Merge
johnrandolph Jun 22, 2022
eeb32b4
Fixes from first review
johnrandolph Jun 22, 2022
cc2f8b6
Move execute to method, keep argv parsing in class
johnrandolph Jun 23, 2022
7afa5b0
Import
johnrandolph Jun 23, 2022
bac9875
Merge remote-tracking branch 'faucet/master'
johnrandolph Jun 23, 2022
fdfda2e
Lint fixes
johnrandolph Jun 23, 2022
45ea044
Up the warnings
johnrandolph Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 37 additions & 4 deletions validator/src/main/java/com/google/daq/mqtt/registrar/LocalDevice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import com.fasterxml.jackson.databind.SerializationFeature;
import com.fasterxml.jackson.databind.util.ISO8601DateFormat;
import com.github.fge.jsonschema.core.exceptions.ProcessingException;
import com.github.fge.jsonschema.core.report.LogLevel;
import com.github.fge.jsonschema.core.report.ProcessingMessage;
import com.github.fge.jsonschema.core.report.ProcessingReport;
import com.github.fge.jsonschema.main.JsonSchema;
import com.google.api.services.cloudiot.v1.model.DeviceCredential;
Expand Down Expand Up @@ -189,6 +191,7 @@ class LocalDevice {
private final String generation;
private final List<DeviceCredential> deviceCredentials = new ArrayList<>();
private final TreeMap<String, Object> siteMetadata;
private final boolean validateMetadata;

private String deviceNumId;

Expand All @@ -197,12 +200,13 @@ class LocalDevice {

LocalDevice(
File siteDir, File devicesDir, String deviceId, Map<String, JsonSchema> schemas,
String generation, Metadata siteMetadata) {
String generation, Metadata siteMetadata, boolean validateMetadata) {
try {
this.deviceId = deviceId;
this.schemas = schemas;
this.generation = generation;
this.siteDir = siteDir;
this.validateMetadata = validateMetadata;
if (siteMetadata != null) {
this.siteMetadata = OBJECT_MAPPER.convertValue(siteMetadata, TreeMap.class);
} else {
Expand All @@ -218,14 +222,28 @@ class LocalDevice {
}
}

LocalDevice(
File siteDir, File devicesDir, String deviceId, Map<String, JsonSchema> schemas,
String generation, Metadata siteMetadata) {
this(siteDir, devicesDir, deviceId, schemas, generation, siteMetadata, false);
}

LocalDevice(
File siteDir, File devicesDir, String deviceId, Map<String, JsonSchema> schemas,
String generation) {
this(siteDir, devicesDir, deviceId, schemas, generation, null);
}

static boolean deviceExists(File devicesDir, String deviceName) {
return new File(new File(devicesDir, deviceName), METADATA_JSON).isFile();
public static void parseMetadataValidateProcessingReport(ProcessingReport report) throws ValidationException {
if (report.isSuccess()) {
return;
}

for (ProcessingMessage msg : report) {
if (msg.getLogLevel().compareTo(LogLevel.ERROR) >= 0) {
throw ValidationException.fromProcessingReport(report);
}
}
}

private void prepareOutDir() {
Expand All @@ -235,6 +253,10 @@ private void prepareOutDir() {
File exceptionLog = new File(outDir, EXCEPTION_LOG_FILE);
exceptionLog.delete();
}

static boolean deviceExists(File devicesDir, String deviceName) {
return new File(new File(devicesDir, deviceName), METADATA_JSON).isFile();
}

public void validateExpected() {
Path relativized = siteDir.toPath().relativize(deviceDir.toPath());
Expand Down Expand Up @@ -265,6 +287,7 @@ public void validateExpected() {
exceptionMap.throwIfNotEmpty();
}


johnrandolph marked this conversation as resolved.
Show resolved Hide resolved
private void deepMergeDefaults(Map<String, Object> destination, Map<String, Object> source) {
for (String key : source.keySet()) {
Object value2 = source.get(key);
Expand All @@ -281,13 +304,19 @@ private void deepMergeDefaults(Map<String, Object> destination, Map<String, Obje
}
}

private Metadata readMetadata() {
private Metadata readMetadataWithValidation(boolean validate) {
File metadataFile = new File(deviceDir, METADATA_JSON);
final JsonNode instance;
try (InputStream targetStream = new FileInputStream(metadataFile)) {
instance = OBJECT_MAPPER.readTree(targetStream);
baseVersion = instance.get("version");
new MessageUpgrader("metadata", instance).upgrade();
ProcessingReport report = schemas.get(METADATA_JSON).validate(instance);
if (validate) {
parseMetadataValidateProcessingReport(report);
}
} catch (ProcessingException | ValidationException e) {
exceptionMap.put(EXCEPTION_VALIDATING, e);
} catch (IOException ioException) {
exceptionMap.put(EXCEPTION_LOADING, ioException);
return null;
Expand Down Expand Up @@ -315,6 +344,10 @@ private Metadata readMetadata() {
return null;
}

private Metadata readMetadata() {
return readMetadataWithValidation(this.validateMetadata);
}

private Metadata readNormalized() {
try {
File metadataFile = new File(outDir, NORMALIZED_JSON);
Expand Down
35 changes: 31 additions & 4 deletions validator/src/main/java/com/google/daq/mqtt/registrar/Registrar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.fasterxml.jackson.databind.util.ISO8601DateFormat;
import com.github.fge.jsonschema.core.load.configuration.LoadingConfiguration;
import com.github.fge.jsonschema.core.load.download.URIDownloader;
import com.github.fge.jsonschema.core.report.ProcessingReport;
import com.github.fge.jsonschema.main.JsonSchema;
import com.github.fge.jsonschema.main.JsonSchemaFactory;
import com.google.api.services.cloudiot.v1.model.Device;
Expand All @@ -29,6 +30,7 @@
import java.io.FilenameFilter;
import java.io.InputStream;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.math.BigInteger;
import java.net.URI;
import java.time.Duration;
Expand Down Expand Up @@ -98,6 +100,8 @@ public class Registrar {
private Duration idleLimit;
private Set<String> cloudDevices;
private Metadata siteMetadata;
private Map<String, Map<String, String>> lastErrorSummary;
private boolean validateMetadata = false;

/**
* Main entry point for registrar.
Expand All @@ -107,6 +111,10 @@ public class Registrar {
public static void main(String[] args) {
ArrayList<String> argList = new ArrayList<>(List.of(args));
Registrar registrar = new Registrar();
executeWithRegistrar(registrar, argList);
}

public static void executeWithRegistrar(Registrar registrar, ArrayList<String> argList) {
Copy link
Collaborator

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 static? Can't it just be a regular class method on Registrar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved this from main() which was static and was looking to be consistent but make the most minimal change. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New attempt, PTAL

try {
boolean processAllDevices = processArgs(argList, registrar);

Expand Down Expand Up @@ -155,6 +163,9 @@ private static boolean processArgs(List<String> argList, Registrar registrar) {
case "-l":
registrar.setIdleLimit(argList.remove(0));
break;
case "-t":
registrar.setValidateMetadata(true);
break;
case "--":
return false;
default:
Expand All @@ -177,6 +188,10 @@ private void setUpdateFlag(boolean update) {
updateCloudIoT = update;
}

private void setValidateMetadata(boolean validateMetadata) {
this.validateMetadata = validateMetadata;
}

private void setFeedTopic(String feedTopic) {
System.err.println("Sending device feed to topic " + feedTopic);
feedPusher = new PubSubPusher(projectId, feedTopic);
Expand All @@ -186,6 +201,10 @@ private void setFeedTopic(String feedTopic) {
}
}

protected Map<String, Map<String, String>> getLastErrorSummary() {
return lastErrorSummary;
}

private void writeErrors() throws Exception {
Map<String, Map<String, String>> errorSummary = new TreeMap<>();
DeviceExceptionManager dem = new DeviceExceptionManager(siteDir);
Expand Down Expand Up @@ -223,9 +242,10 @@ private void writeErrors() throws Exception {
String version = Optional.ofNullable(System.getenv(UDMI_VERSION_KEY)).orElse("unknown");
errorSummary.put(VERSION_KEY, Map.of(VERSION_MAIN_KEY, version));
OBJECT_MAPPER.writeValue(summaryFile, errorSummary);
lastErrorSummary = errorSummary;
}

private void setSitePath(String sitePath) {
protected void setSitePath(String sitePath) {
Preconditions.checkNotNull(SCHEMA_NAME, "schemaName not set yet");
siteDir = new File(sitePath);
summaryFile = new File(siteDir, REGISTRATION_SUMMARY_JSON);
Expand Down Expand Up @@ -638,7 +658,7 @@ private Map<String, LocalDevice> loadDevices(File siteDir, File devicesDir,
localDevices.computeIfAbsent(
deviceName,
keyName -> new LocalDevice(siteDir, devicesDir, deviceName, schemas, generation,
siteMetadata));
siteMetadata, validateMetadata));
try {
localDevice.loadCredentials();
} catch (Exception e) {
Expand All @@ -659,15 +679,15 @@ private Map<String, LocalDevice> loadDevices(File siteDir, File devicesDir,
return localDevices;
}

private void setProjectId(String projectId) {
protected void setProjectId(String projectId) {
if (NO_SITE.equals(projectId) || projectId == null) {
return;
}
this.projectId = projectId;
initializeCloudProject();
}

private void setToolRoot(String toolRoot) {
protected void setToolRoot(String toolRoot) {
schemaBase = new File(toolRoot, SCHEMA_BASE_PATH);
File[] schemaFiles = schemaBase.listFiles(file -> file.getName().endsWith(SCHEMA_SUFFIX));
for (File schemaFile : Objects.requireNonNull(schemaFiles)) {
Expand Down Expand Up @@ -705,6 +725,9 @@ private void loadSiteMetadata() {

File siteMetadataFile = new File(siteDir, SITE_METADATA_JSON);
try (InputStream targetStream = new FileInputStream(siteMetadataFile)) {
// At this time, do not validate the Metadata schema because, by its nature of being
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused by this comment -- it says "do not validate" and then the code right after is validate()... ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but we ignore the return value, the report.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still confused -- if it's ignored then why is it calling validate()? Do you mean something like "At this time, only validate the structure of the Metadata schema because it's only a partial overlay, and defer any in-depth validation until after merge?"

// a partial overlay on each device Metadata, this Metadata will likely be incomplete
// and fail validation.
schemas.get(METADATA_JSON).validate(OBJECT_MAPPER.readTree(targetStream));
} catch (FileNotFoundException e) {
return;
Expand All @@ -720,6 +743,10 @@ private void loadSiteMetadata() {
}
}

protected Map<String, JsonSchema> getSchemas() {
return schemas;
}

class RelativeDownloader implements URIDownloader {

@Override
Expand Down
95 changes: 95 additions & 0 deletions validator/src/main/java/com/google/daq/mqtt/registrar/RegistrarTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package com.google.daq.mqtt.registrar;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.github.fge.jsonschema.core.exceptions.ProcessingException;
import com.github.fge.jsonschema.core.report.LogLevel;
import com.github.fge.jsonschema.core.report.ProcessingMessage;
import com.github.fge.jsonschema.core.report.ProcessingReport;
import com.github.fge.jsonschema.main.JsonSchema;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Map;
import java.util.TreeMap;
import org.junit.Test;
import udmi.schema.Metadata;

public class RegistrarTest {

public static final String SCHEMA_BASE_PATH = "schema";
johnrandolph marked this conversation as resolved.
Show resolved Hide resolved
private static final String METADATA_JSON = "metadata.json";
private static final String PROJECT_ID = "unit-testing";
private static final String SITE_PATH = "../sites/udmi_site_model";
private static final String TOOL_ROOT = "../";

private static final String SYSTEM_LOCATION_SITE = "ZZ-TRI-FECTA";
private static final String DEVICE_NAME = "AHU-1";
private ObjectMapper mapper = new ObjectMapper();

public class RegistrarUnderTest extends Registrar {
protected JsonSchema getJsonSchema(String schemaName) {
return getSchemas().get(schemaName);
}
}

private void assertErrorSummaryValidateSuccess(Map<String, Map<String, String>> summary) {
if (summary == null) {
johnrandolph marked this conversation as resolved.
Show resolved Hide resolved
return;
}
if (summary.get("Validating") == null) {
return;
}
if (summary.get("Validating").size() == 0) {
return;
}
fail(summary.get("Validating").toString());
}

private void assertErrorSummaryValidateFailure(Map<String, Map<String, String>> summary) {
if ((summary == null) || (summary.get("Validating") == null)) {
fail("Error summary for Validating key is null");
}
if (summary.get("Validating").size()==0) {
fail("Error summary for Validating key is size 0");
}
}

private RegistrarUnderTest getRegistrarUnderTest() {
RegistrarUnderTest registrar = new RegistrarUnderTest();
registrar.setSitePath(SITE_PATH);
registrar.setProjectId(PROJECT_ID);
registrar.setToolRoot(TOOL_ROOT);
return registrar;
}

@Test public void metadataValidateSuccessTest() {
RegistrarUnderTest registrar = getRegistrarUnderTest();

ArrayList<String> argList = new ArrayList<String>();
argList.add("-s");
argList.add(SITE_PATH);
registrar.executeWithRegistrar(registrar, argList);
assertErrorSummaryValidateSuccess(registrar.getLastErrorSummary());
}

@Test public void metadataValidateFailureTest() {
RegistrarUnderTest registrar = getRegistrarUnderTest();

ArrayList<String> argList = new ArrayList<String>();
argList.add("-t");
argList.add("-s");
argList.add(SITE_PATH);
registrar.executeWithRegistrar(registrar, argList);
assertErrorSummaryValidateFailure(registrar.getLastErrorSummary());
}

}
9 changes: 9 additions & 0 deletions validator/src/test/java/com/google/daq/mqtt/validator/ValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ public class ValidatorTest {
validator.setSiteDir(SITE_DIR);
}

@Test
public void emptySystemBlock() {
MessageBundle bundle = getMessageBundle("event", "pointset", new PointsetEvent());
bundle.message.remove("system");
validator.validateMessage(bundle);
MetadataReport report = getMetadataReport();
assertEquals("One error device", 1, report.errorDevices.size());
}

@Test
public void emptyPointsetEvent() {
MessageBundle bundle = getMessageBundle("event", "pointset", new PointsetEvent());
Expand Down