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

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 15 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
39 changes: 34 additions & 5 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 @@ -281,7 +303,7 @@ 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)) {
Expand All @@ -294,7 +316,10 @@ private Metadata readMetadata() {
}

try {
schemas.get(METADATA_JSON).validate(instance);
ProcessingReport report = schemas.get(METADATA_JSON).validate(instance);
if (validate) {
parseMetadataValidateProcessingReport(report);
}
} catch (ProcessingException | ValidationException e) {
exceptionMap.put(EXCEPTION_VALIDATING, e);
}
Expand All @@ -315,6 +340,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
91 changes: 58 additions & 33 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,9 @@ public class Registrar {
private Duration idleLimit;
private Set<String> cloudDevices;
private Metadata siteMetadata;
private Map<String, Map<String, String>> lastErrorSummary;
private boolean validateMetadata = false;
private List<String> deviceList;

/**
* Main entry point for registrar.
Expand All @@ -107,33 +112,11 @@ public class Registrar {
public static void main(String[] args) {
ArrayList<String> argList = new ArrayList<>(List.of(args));
Registrar registrar = new Registrar();
try {
boolean processAllDevices = processArgs(argList, registrar);

if (registrar.schemaBase == null) {
registrar.setToolRoot(null);
}

registrar.loadSiteMetadata();

if (processAllDevices) {
registrar.processDevices();
} else {
registrar.processDevices(argList);
}

registrar.writeErrors();
registrar.shutdown();
} catch (ExceptionMap em) {
ExceptionMap.format(em, ERROR_FORMAT_INDENT).write(System.err);
throw new RuntimeException("mapped exceptions", em);
} catch (Exception ex) {
ex.printStackTrace();
throw new RuntimeException("main exception", ex);
}
processArgs(argList, registrar);
registrar.execute();
}

private static boolean processArgs(List<String> argList, Registrar registrar) {
public static void processArgs(List<String> argList, Registrar registrar) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not sure why this is static? The general adage with static methods for Java is to avoid them unless they need to be static for some reason. I think this comes down to the issue that you can't mock/override a static method, so it makes future testing/extension a problem sometimes. Not likely in this case, but just on general practice. E.g., I would expect this to be just a regular method on the Registrar class (no need to pass in the explicit parameter). Anyway, that's mainly the Java-style that I've always used, but it's been so long that I'm not 100% sure where it came from. I checked the Java style guide and it doesn't say anything about it. Doesn't really matter -- I also optimize for "least number of words" and if you made it a class method, all references to "registrar." go away reducing the overall footprint of the class -- and there's no world in which you would call this method without a registrar class!

while (argList.size() > 0) {
String option = argList.remove(0);
switch (option) {
Expand All @@ -155,17 +138,39 @@ 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;
break;
default:
if (option.startsWith("-")) {
throw new RuntimeException("Unknown cmdline option " + option);
}
// Add the current non-option back into the list and use it as device names list.
argList.add(0, option);
return false;
registrar.setDeviceList(argList);
return;
}
}
return true;
}

public void execute() {
try {
if (schemaBase == null) {
setToolRoot(null);
}
loadSiteMetadata();
processDevices();
writeErrors();
shutdown();
} catch (ExceptionMap em) {
ExceptionMap.format(em, ERROR_FORMAT_INDENT).write(System.err);
throw new RuntimeException("mapped exceptions", em);
} catch (Exception ex) {
ex.printStackTrace();
throw new RuntimeException("main exception", ex);
}
}

private void setIdleLimit(String option) {
Expand All @@ -177,6 +182,14 @@ private void setUpdateFlag(boolean update) {
updateCloudIoT = update;
}

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

private void setDeviceList(List<String> deviceList) {
this.deviceList = deviceList;
}

private void setFeedTopic(String feedTopic) {
System.err.println("Sending device feed to topic " + feedTopic);
feedPusher = new PubSubPusher(projectId, feedTopic);
Expand All @@ -186,6 +199,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 +240,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 @@ -259,7 +277,7 @@ private String getGenerationString() {
}

private void processDevices() {
processDevices(null);
processDevices(this.deviceList);
}

private void processDevices(List<String> devices) {
Expand Down Expand Up @@ -638,7 +656,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 +677,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 +723,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 +741,10 @@ private void loadSiteMetadata() {
}
}

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

class RelativeDownloader implements URIDownloader {

@Override
Expand Down
Loading