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

Adds schema creating csv schema with View #195

Merged
merged 1 commit into from
Apr 25, 2020

Conversation

Qnzvna
Copy link
Contributor

@Qnzvna Qnzvna commented Apr 23, 2020

No description provided.

@cowtowncoder
Copy link
Member

@Qnzvna this looks very useful, thank you! I think I have some comments wrt implementation, but in general I think I like the idea.

Just one practical thing before I can merge this: unless I have asked for CLA:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

I would one before first contributions (but only then; future contributions can all be under existing CLA then).
Usually the easiest way is to print, fill & sign, scan/photo, email to info at fasterxml dot com.

@@ -460,6 +487,18 @@ protected void _addSchemaProperties(SerializerProvider ctxt, CsvSchema.Builder b
BeanDescription beanDesc = ctxt.introspectBeanDescription(pojoType);
final AnnotationIntrospector intr = ctxt.getAnnotationIntrospector();
for (BeanPropertyDefinition prop : beanDesc.findProperties()) {
if (view != null) {
boolean viewVisible = false;
for(Class<?> propView : prop.findViews()) {
Copy link
Member

Choose a reason for hiding this comment

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

Needs null check, findViews() can return null. Also note that in databind BeanDeserializerFactory method addBeanProps(), there is:

                Class<?>[] views = propDef.findViews();
                if (views == null) {
                    views = beanDesc.findDefaultViews();
                }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated.

if (view != null) {
boolean viewVisible = false;
for(Class<?> propView : prop.findViews()) {
if (propView.equals(view)) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sufficient: Views actually check inheritance too (so, say, CharSequence as inclusion would be acceptable for View String).
You might be able to use databind's ViewMatcher for checks:

public static ViewMatcher construct(Class<?>[] views) { }

and then calling method:

        public boolean isVisibleForView(Class<?> activeView) { ...}

seems like the proper (and still simple) way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated also.

@Qnzvna Qnzvna force-pushed the csv-schema-with-view branch from 298bca6 to 07c084c Compare April 25, 2020 09:20
@Qnzvna
Copy link
Contributor Author

Qnzvna commented Apr 25, 2020

I've signed the CLA before when providing different contributions.

@cowtowncoder
Copy link
Member

@Qnzvna Ahh! I did find the CLA from 2017. Thank you!

@cowtowncoder cowtowncoder merged commit a9d7e31 into FasterXML:master Apr 25, 2020
@cowtowncoder cowtowncoder added this to the 2.11.0 milestone Apr 25, 2020
@cowtowncoder
Copy link
Member

Excellent: I backport this in 2.11 branch and this will just make it in 2.11.0 thanks to timing.

Thank you again for contributing this new feature!

@Qnzvna
Copy link
Contributor Author

Qnzvna commented Apr 26, 2020

Thanks for merging so quickly.

@Qnzvna Qnzvna deleted the csv-schema-with-view branch April 26, 2020 17:32
@cowtowncoder
Copy link
Member

@Qnzvna and 2.11.0 is actually out now! Perfect timing. :)

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.

2 participants