Binary property support #171

ctrimble · 2014-04-15T20:42:48Z

This feature will provide support for binary properties, using the media block, as specified by the JSON Hyper Schema specification. Currently, this PR only includes the test case for the functionality.

Blocks declared in the form:

"binaryProperty": {
  "type": "string",
  "media": {
    "binaryEncoding": "base64"
  }
}

will create properties of this form in the POJOs:

byte[] binaryProperty;

public byte[] getBinaryProperty() {
  return this.binaryProperty;
}

public void setBinaryProperty( byte[] binaryProperty ) {
  this.binaryProperty = binaryProperty;
}

Media blocks without a binaryEncoding will be ignored.

ctrimble · 2014-04-15T22:36:04Z

jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/rules/TypeRule.java

@@ -105,6 +105,9 @@ public JType apply(String nodeName, JsonNode node, JClassContainer jClassContain
        if (node.has("format")) {
            type = ruleFactory.getFormatRule().apply(nodeName, node.get("format"), type, schema);
        }
+        else if(propertyTypeName.equals("string") && node.has("media")) {
+            type = ruleFactory.getMediaRule().apply(nodeName, node.get("media"), type, schema);
+        }


@joelittlejohn does this seem like the correct way for format and media to interact, when they are both present?

Yes, I think so. It'll minimize backwards-compatibility problems in the obscure case that someone is using both format and media. I think you could make an argument for either to win out in this case but I think you've taken the right path.

ctrimble · 2014-04-16T16:45:40Z

I think this change is almost complete. There should probably be some kind of testing for the interplay between this feature and things like format and default, just to pin down the behavior.

ctrimble · 2014-04-16T16:49:47Z

pom.xml

+                <artifactId>commons-codec</artifactId>
+                <version>1.9</version>
+                <scope>test</scope>
+            </dependency>


Be aware, I added a dependency to commons-codec, in the test scope.

ctrimble · 2014-04-18T16:22:50Z

@joelittlejohn I think this change is done. There are a lot of commits, so a squash is probably in order. If you think this PR looks good, I will squash these just before you pull, so your history is clean.

This is what I changed recently:

Added JavaDocs
Changed the base32 example to a quoted-printable example. This encoding is actually defined in RFC 2045, so people might run into this.
Added testing for format interaction
Added a test for default interaction. I added no support for defaults, as it looked like this might increase the scope of this change too much. The code just ignores default values right now.

joelittlejohn · 2014-04-21T21:23:54Z

This looks brilliant Christian. Sorry for the delay, I've been out of town for the last few days. I must say your attention to detail and sympathy with the existing conventions, structure and layout of this project are exceptional. A couple of tiny format issues:

There's a tab on jsonschema2pojo-integration-tests/src/test/resources/schema/media/mediaProperties.json:7
else should not be on a new line on jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/rules/TypeRule.java:108

I've given this a review (git diff master...feature_binary-fields) and I don't see any issues with your implementation. Feel free to squash and rebase against master and I'll merge.

If you have any feedback on the overall structure and ease of changing this code then please don't hesitate to comment.

Cheers!

- add a new rule for media - added media rule to the rule factory - added code to type rule to trigger the media rule - added test cases for binary fields.

ctrimble · 2014-04-22T01:13:30Z

@joelittlejohn Thanks! I think this change is good to go. I squashed and rebased the commit, so this should be ready to merge.

Overall, it has not been hard to navigate the project and find things I am looking for. There were two things that did cause me problems:

This project does not currently build with the Java 8 JDK on OSX. This seems to be due to JavaDoc errors busting the build, but I did not spend much time on root cause. I will get a ticket submitted for this (and maybe a PR.) I didn't notice that this was fixed on master 3 hours ago.
This project forks when aggregating JavaDocs. This kills my OSS setup, as I use mvn -s <SETTINGS_FILE> to turn off my work's mirror and isolate my local repository. Maven does not handle this properly and my settings file gets ignored. Long story short, I have to reset my .m2/settings.xml file to work with your project. This is maven's problem, not yours. Perhaps it is time for me to submit a patch to Maven.

Otherwise, this project has been a pleasure to work with. Thanks so much for your feedback during the addition of this feature. There is nothing worse than preparing a patch, only to rework it again during the PR.

-Christian

joelittlejohn · 2014-04-22T07:33:05Z

Coincidentally I was actually looking into those JDK 8 failures last night.
I fixed the javadoc issue and another strange quirk which caused
compilation of the Gradle plugin to fail.

The current master should compile on JDK 8.
On 22 Apr 2014 02:13, "Christian Trimble" [email protected] wrote:

@joelittlejohn https://github.com/joelittlejohn Thanks! I think this
change is good to go. I squashed and rebased the commit, so this should be
ready to merge.

Overall, it has not been hard to navigate the project and find things I am
looking for. There were two things that did cause me problems:

This project does not currently build with the Java 8 JDK on OSX.
This seems to be due to JavaDoc errors busting the build, but I did not
spend much time on root cause. I will get a ticket submitted for this (and
maybe a PR.)

This project forks when aggregating JavaDocs. This kills my OSS
setup, as I use mvn -s <SETTINGS_FILE> to turn off my work's mirror
and isolate my local repository. Maven does not handle this properly and my
settings file gets ignored. Long story short, I have to reset my
.m2/settings.xml file to work with your project. This is maven's
problem, not yours. Perhaps it is time for me to submit a patch to Maven.

Otherwise, this project has been a pleasure to work with. Thanks so much
for your feedback during the addition of this feature. There is nothing
worse than preparing a patch, only to rework it again during the PR.

-Christian

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/171#issuecomment-40994359
.

Binary property support

ctrimble reviewed Apr 15, 2014
View reviewed changes

ctrimble reviewed Apr 16, 2014
View reviewed changes

joelittlejohn added this to the 0.4.2 milestone Apr 17, 2014

added binary property support

5ceee94

- add a new rule for media - added media rule to the rule factory - added code to type rule to trigger the media rule - added test cases for binary fields.

joelittlejohn added a commit that referenced this pull request Apr 22, 2014

Merge pull request #171 from ctrimble/feature_binary-fields

b16da4c

Binary property support

joelittlejohn merged commit b16da4c into joelittlejohn:master Apr 22, 2014

ctrimble deleted the feature_binary-fields branch April 25, 2014 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary property support #171

Binary property support #171

ctrimble commented Apr 15, 2014

ctrimble Apr 15, 2014

joelittlejohn Apr 15, 2014

ctrimble commented Apr 16, 2014

ctrimble Apr 16, 2014

ctrimble commented Apr 18, 2014

joelittlejohn commented Apr 21, 2014

ctrimble commented Apr 22, 2014

joelittlejohn commented Apr 22, 2014

Binary property support #171

Binary property support #171

Conversation

ctrimble commented Apr 15, 2014

ctrimble Apr 15, 2014

Choose a reason for hiding this comment

joelittlejohn Apr 15, 2014

Choose a reason for hiding this comment

ctrimble commented Apr 16, 2014

ctrimble Apr 16, 2014

Choose a reason for hiding this comment

ctrimble commented Apr 18, 2014

joelittlejohn commented Apr 21, 2014

ctrimble commented Apr 22, 2014

joelittlejohn commented Apr 22, 2014