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

Add 'formatTypeMapping' config option to allow overriding types used for formats #923

Merged

Conversation

jrehwaldt
Copy link
Contributor

@jrehwaldt jrehwaldt commented Sep 18, 2018

This commit introduces a new option formatTypeMapping, which maps from format
(e.g. 'uri') to type (e.g. 'java.net.URI'). The current base types and configurations
are left as-is. This means, that it is still possible to set 'useJodaDate' or 'dateType'
as before. Alternatively, a mapping 'date' > 'org.joda.time.LocalDate' achieves the same
output.

With this change it is also possible to map non-standard formats to types. This logic is
intentional, but not recommended to users as it invalidates the schema.

This (partly) fixes gh-910 and is an alternative to gh-918.

This commit introduces a new option `formatTypeMapping`, which maps from format
(e.g. 'uri') to type (e.g. 'java.net.URI'). The current base types and configurations
are left as-is. This means, that it is still possible to set 'useJodaDate' or 'dateType'
as before. Alternatively, a mapping 'date' > 'org.joda.time.LocalDate' achieves the same
output.

With this change it is also possible to map non-standard formats to types. This logic is
intentional, but not recommended to users as it invalidates the schema.
@jrehwaldt
Copy link
Contributor Author

I'd actually go as far as to remove the options dateType, dateTimeType, timeType as well as useJodaDateTime, useJodaLocalDates, useJodaLocalTime. Anyway, this is entirely optional and the PR as-is preserved backwards compatibility.

@joelittlejohn
Copy link
Owner

Thanks. Could you explain your use-case?

@jrehwaldt
Copy link
Contributor Author

Currently there are 14 formats defined by the spec. For each this library chose a suitable implementation in Java. Seeing the 2*3 options to configure dates (useJodaXxx being redundant as it can actually be set using xxxType) shows that there is a need to change these types already. Watching only the date-related options it is not clear what the interplay of the options is and how they override one another.

A use case is ip-address and ipv6 resolve to String, while Java provides the class java.net.Inet4Address and Inet6Address for exactly that purpose. Another example is regex as Pattern. While technically correct it is sometimes not desired to perform pattern matching directly with the provided data, considering Regular expression Denial of Service attacks. Now I am not advocating to change the current mapping, but rather to allow users in an easy and configurable way to override the default mapping from format to type.

Having said that my use case is still the edge case gh-910. Nonetheless, I imagine this solution is easier maintainable in the long run than the current 6 mutually exclusive date properties together with what might come in the future -- issues and json-schema drafts.

@joelittlejohn
Copy link
Owner

I think you have made a strong case for this. I'll review the changes with the intention of including this in the next release.

@joelittlejohn joelittlejohn changed the title Implement configuration to override types for formats. Add 'formatTypeMapping' config option to allow overriding types used for formats Sep 19, 2018
@joelittlejohn joelittlejohn added this to the 1.0.0-beta1 milestone Sep 19, 2018
@@ -497,6 +497,12 @@ <h3>Parameters</h3>
</td>
<td align="center" valign="top">No (default <code>OS</code>)</td>
</tr>
<tr>
<td valign="top">formatTypeMapping</td>
<td valign="top">A mapping from format identifier (e.g. 'uri') to Java type (e.g. 'java.net.URI').
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add an example to the description that shows what format the value should have?

@joelittlejohn
Copy link
Owner

@jrehwaldt It looks like you're finished on this PR. Okay for me to merge?

Thanks again for your contributions. You've made changes very carefully and added tests where appropriate, which I really appreciate.

@jrehwaldt
Copy link
Contributor Author

Ready for merge, yes. Thank you.

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

Successfully merging this pull request may close these issues.

Generate URL when format=uri and pattern=https?://
2 participants