eclipse-archived / californium.core

Californium project
86 stars 69 forks source link

Add CBOR type specification to the MediaTypeRegistry #54

Closed ryanmeador closed 8 years ago

ryanmeador commented 8 years ago

The types in the MediaTypeRegistry appear to be only those described in the initial RFC. The RFC creates an IANA registry of types for use going forward, which are listed here: http://www.iana.org/assignments/core-parameters/core-parameters.xhtml#content-formats

CBOR was the only one missing as far as I can tell.

sophokles73 commented 8 years ago

Hi @ryanmeador,

thanks for submitting the Pull Request. However, in order for us to be able to merge it, you will need to sign an Eclipse Contributor License Agreement and the include a Signed-off-by footer in your commit message(s) using the same email address you signed the CLA for.

ryanmeador commented 8 years ago

I signed the agreement and pushed a new commit with the signoff. I think it's all set now.

mkovatsc commented 8 years ago

Thanks for adding this!

In the long run, there will be a potentially very long list (e.g., see http://svn.tools.ietf.org/svn/wg/core/mediatypes.txt). Probably, we should look into some code generation to to at least be in sync with the IANA registry.

Was there a blocking point when not having the definition in californium-core?

ryanmeador commented 8 years ago

Hi @mkovatsc. I'm not quite sure what that link of media types is... the numbers don't correspond to the content formats in the IANA registry I linked to earlier or the CoAP RFC. If they really are something that needs to be supported, code generation is definitely the way to go! I've had good luck in the past generating code like that using the JavaPoet library, FWIW.

This PR was inspired by needing the CBOR type for an application I was writing. As a stopgap I just passed the number 60 into CoapClient.post() etc, but I wanted to do it "the right way" by adding it to the MediaTypeRegistry.

sophokles73 commented 8 years ago

@mkovatsc,

I would also be interested to learn about the relation between the link provided by you (containing the dozens of content types) and the official IANA registered content types for CoAP (a handful).

We already have several content type definitions in the MediaTypeRegistry that are not contained in the IANA registry (yet?). I wonder where the corresponding codes come from, e.g. 45 for application/atom+xml ...

Any hints would be highly appreciated :-)

mkovatsc commented 8 years ago

Klaus Hartke automatically created the SVN document for experimentation with other media types. Note that it is purely experimental so far!

The other entries in Californium's registry come from earlier CoAP drafts, which had more initial entries. Probably we should clean them and only have the ones in the IANA registry. In additional, we could have an experimental registry in an inner public class "X" or therelike. Real experimental IDs should be defined in the application without changes to the MediaTypeRegistry.

mkovatsc commented 8 years ago

I would merge this now or, if you like, you could sync it with IANA first :)

sophokles73 commented 8 years ago

+1 for merging CBOR type.

I think we should remove the non-official types from MediaTypeRegistry in order to prevent confusion. But this should be done in a separate commit. Can you do that, @mkovatsc?

ryanmeador commented 8 years ago

May I suggest making MediaTypeRegistry.add() public, to enable use for experimental types?

mkovatsc commented 8 years ago

For a useful public add(), we also need to add the isPrintable boolean to the entries. This could be done through an inner class. In short: yes, but we need some more changes.

@ryanmeador Interested in doing this? :) You can simply add more commits to this pull request.

ryanmeador commented 8 years ago

I'd be happy to write the code and add it to this PR, but I don't know if I have the necessary context. What is the isPrintable boolean? Also, do unit tests exist for this class?

mkovatsc commented 8 years ago

The isPrintable() method currently uses a switch/case based on the member constants. For a dynamic MediaTypeRegistry, this information must come from the registry hashmap (similar to toFileExtension() or toString()).

I am not aware of any unit tests out of my head, but I don't even see how these should work without creating a secondary registry ;)

ryanmeador commented 8 years ago

Oh. I didn't even noticed that isPrintable thing, and thus I left it out of this PR! I should have included CBOR in that switch. I can refactor it to use a MediaType class (or maybe even an enum?) to represent the data, and include an isPrintable boolean on that class. I should also be able to create some unit tests to ensure I don't break anything, assuming I can get your test framework up and running.

I'll try to tackle this tomorrow afternoon (UTC-5); my employer gives a few hours on Friday afternoons for projects like this.

mkovatsc commented 8 years ago

Okay, great! Best amend the current commit first with the CBOR entry in isPrintible(). Then add one commit for the clean-up of types not in the IANA registry, and one for the public add() interface. Thanks!

ryanmeador commented 8 years ago

Updated PR with that fix. I think it makes more logical sense for me to start a new PR for the refactoring, so feel free to merge this one if you'd like.