[JAVA-4545] Introduce convention to disable getters/setters during POJO serialization Created: 22/Mar/22  Updated: 09/Oct/23

Status: Backlog
Project: Java Driver
Component/s: POJO
Affects Version/s: None
Fix Version/s: None

Type: New Feature Priority: Major - P3
Reporter: Robert Fox Assignee: Ross Lawley
Resolution: Unresolved Votes: 1
Labels: external-user
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
duplicates JAVA-3220 Protected and package private fields ... Backlog
Quarter: FY24Q4
Documentation Changes Summary:

1. What would you like to communicate to the user about this feature?
2. Would you like the user to see examples of the syntax and/or executable code and its output?
3. Which versions of the driver/connector does this apply to?


 Description   

Currently, there isn't a nice way for users of the POJO persistence mechanism to prevent property models from automatically incorporating getXXX methods as properties.  I'm suggesting the introduction of a FieldsOnlyConvention (or something similar) that will allow users of the Java driver to enforce methods are never used globally (rather than depend on developers correctly annotating every single method with @BsonIgnore)

 

For my company, this has been a source of bugs, data leakage, and data duplication.  Potentially a security concern.  But mostly data duplication.  It's easy for a developer to add a "getXYZ" method that provides a new view of the underlying data (without realizing that the POJO will end up with lots of duplicated data when stored as a MongoDB document. 

There is no great way for developers in user land to develop a solution for this (I've tried). 



 Comments   
Comment by Jakob Stiel [ 11/Jan/23 ]

Hi,

are there any updates or possibly a good solution on this issue?

Comment by Robert Fox [ 07/Jun/22 ]

Hi there I see that this feature has moved to "Scheduled"!  Wow great news. 

The only outstanding question I had regards to your last comments: will the Java driver respect the field types as declared?  Or must you use generic / abstract / interface classes like "Map" instead of "ConcurrentHashMap"?  All of "the usual suspects" tend to have a default constructor.  I was wondering if it's possible to ensure this convention respects the collection types used in the Java class files.

Comment by Ross Lawley [ 17/May/22 ]

Apologies robert@unitydynamics.com for the delay in response,

If I understand your second bullet, you're essentially "pruning" property models if they are not backed by a field; but my question would be, if a "setter" exists for a given field, will it be called during de-serialization (from MongoDB to Java)? And if a getter exists, will be be called before serialization (Java to MongoDB)?

So if you use this convention then the field is the source of true and any getter / setters will not be used.

Also a general question... would this new convention be compatible (or incompatible) with any other conventions in particular?
Conventions.ANNOTATION_CONVENTION, Conventions.USE_GETTERS_FOR_SETTERS

Thats correct some conventions will override previous conventions - so its not compatible with: USE_GETTERS_FOR_SETTERS or SET_PRIVATE_FIELDS_CONVENTION.
It is compatible with the ANNOTATION_CONVENTION for example @BsonId and @BsonProperty are valid for fields.

Its important to note that ordering is important with conventions - they are applied to the ClassModel in order so mixing opposing conventions may not produce the desired effect.

I have a peculiar need for the "USE_GETTERS_FOR_SETTERS" convention. In my case I want to preserve the implementation class of collections (example: map must be 'ConcurrentHashMap'). By default the Java driver will replace all of our Maps with java.util.HashMap. But we can force the right behavior by making the map final and instantiating it in the constructor IF we specify USE_GETTERS_FOR_SETTERS. What's really strange / confusing here is that we don't actually need a getter for the map–the driver uses the existing map field!

Now that is slightly different issue, we do test using specific types eg: ConcreteCollectionsModel however, all the getters and setters use the same type. The PojoBuilderHelper checks setters first as they generally accept wider types. I'll look into using the Field type explicitly.

Ross

Comment by Robert Fox [ 30/Apr/22 ]

Hi Ross, thank you for the work on this!

In regards to your first bullet / note on discovery... for us it's not a deal breaker to avoid serialization of private members without accessors.

If I understand your second bullet, you're essentially "pruning" property models if they are not backed by a field; but my question would be, if a "setter" exists for a given field, will it be called during de-serialization (from MongoDB to Java)? And if a getter exists, will be be called before serialization (Java to MongoDB)?

 

Also a general question... would this new convention be compatible (or incompatible) with any other conventions in particular?

Conventions.ANNOTATION_CONVENTION, Conventions.USE_GETTERS_FOR_SETTERS

Are the two we're using now. 

I have a peculiar need for the "USE_GETTERS_FOR_SETTERS" convention.  In my case I want to preserve the implementation class of collections (example: map must be 'ConcurrentHashMap').  By default the Java driver will replace all of our Maps with java.util.HashMap.  But we can force the right behavior by making the map final and instantiating it in the constructor IF we specify USE_GETTERS_FOR_SETTERS.  What's really strange / confusing here is that we don't actually need a getter for the map–the driver uses the existing map field!

Could you tell me if this kind of behavior would be consistent with the new convention?  Willing to do testing on my end (but not sure I'm capable of building from code)

 

Comment by Ross Lawley [ 27/Apr/22 ]

Hi robert@unitydynamics.com,

I've been working on a convention that gets and sets fields directly and before merging I wanted to double check it meets your requirements.

Things to note are:

  • It uses the ClassModelBuilders property discovery mechanism - so only properties that have either of a public field, getter, or setter are discoverable. The following tests highlight what is and isn't serialized / deserialized.
  • It removes any property models that aren't backed by a field - I'm not sure if this behaviour should be put into a separate convention.

I wanted to double check if that would meet your needs and if you had any feedback before merging.

All the best,

Ross

Comment by Robert Fox [ 22/Mar/22 ]

Yes I saw through my debugger that the PropertyMetaData seemed to have information that I needed. 

I think you have it!: 1) a way to gather PropertyModels using only non-transient and non-static fields (with any level of access), and 2) simply persisting those fields without execution of getter/setter on serialization/de-serialization.  Conceptually equivalent (I believe) to disabling PropertyModel detection via methods.

This would be fantastic from a security-review perspective as well.  To be able to say definitively: my "serialization routine does not execute any user code" (other than default constructors) would save time we spend ensuring that our code does not contain any side effects.  We have a ton of different model objects.  In those very rare circumstances that we would want to run custom code on de/serialization we would probably just make a Codec.

Thank you, sir, for your consideration!

Robert

 

Comment by Ross Lawley [ 22/Mar/22 ]

Interesting,

So it wants to be something similar to the [ConventionSetPrivateFieldImpl|(https://github.com/mongodb/mongo-java-driver/blob/r4.5.0/bson/src/main/org/bson/codecs/pojo/ConventionSetPrivateFieldImpl.java] which indirectly has access to the PropertyMetaData.

Just to clarify, the ideal convention for your usecase would be to use the fields only and ignore all getters and setters? and the fields may or may not be public?

Many thanks,

Ross

Comment by Robert Fox [ 22/Mar/22 ]

I gave it a shot.  Along the lines of this answer on StackOverflow.  The initial idea was to use reflection and then simply remove anything that was not a "field" from the ClassModelBuilder. 

HOWEVER, the situation is complicated by the fact that (of course) you may have both a field "public Integer stuff" and a method "public Integer getStuff()" --and then of course I wouldn't want to remove the property... but somehow magically enforce that the getter was not used in serialization. 

I think that the required fix is upstream of the Convention.  When discovering properties?

Comment by Ross Lawley [ 22/Mar/22 ]

Hi robert@unitydynamics.com,

Thanks for the ticket. Conventions are a good layer / level to do this, did you end up writing your own convention?

Ross

Generated at Thu Feb 08 09:02:21 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.