Filter out sythetic fields from FieldQueryMapEncoder #840
Conversation
| return new ObjectParamMetadata( | ||
| Arrays.stream(type.getDeclaredFields()) | ||
| .filter(field -> !field.isSynthetic()) | ||
| .peek(field -> field.setAccessible(true)) |
There was a problem hiding this comment.
Technically, this violates the non-interference contract, but I don't see any actual issues with it.
kdavisk6
added
the
ready to merge
| List<Field> fields = new ArrayList<Field>(); | ||
| for (Field field : type.getDeclaredFields()) { | ||
| if (!field.isAccessible()) { | ||
| field.setAccessible(true); |
There was a problem hiding this comment.
@rage-shadowman we already do it
There was a problem hiding this comment.
No, that's how it's supposed to be done. It's also a per-instance thing, so the next guy who gets the declared fields from the class will have them inaccessible. I was talking about the stream usage modifying an object inside the stream.
I believe the stream non-interference contracts are about leaving the original object alone, so peek is only intended for use in statistics gathering and logging (or other such non-interfering things), and you're supposed to use map (and create a new object for the map to return rather than modifying objects in-line which is a side-effect). However, in this particular usage, I don't see anything wrong with violating the contract since you are the sole creator, maintainer, and user of the stream and all its objects.
There was a problem hiding this comment.
But setAccessible is void or am I missing something obvious?
There was a problem hiding this comment.
It may be void, but it still mutates the Field object internally, making it accessible. From the stream's point of view, that's a side-effect (it's the effect you intended, but to the stream, it's a side-effect that violates the non-interference contract by modifying the stream's objects). Also, since it is a void method, you can't do this in a stream without violating that contract (you'd have to use a for loop instead, thereby making it an effect, rather than a side-effect).
Like I said, I don't think it's an issue since this is all done in 1 thread, and it doesn't have any side-effects from the perspective of anyone outside of this method (if the side-effect modified any objects that you didn't just create yourself, I might feel differently). This is something you should keep in mind whenever you use streams.
Please excuse me if you already know this, but more generally, you need to be aware of all your side-effects in order to know how complex your code will be in a multi-threaded context (the more complex, the more likely you have threading bugs that you just haven't seen yet and won't easily be able to debug -- or even reproduce -- when you do see them). Making code that is incapable of side-effects (immutable objects and unmodifiiable collections everywhere, no arrays, no output parameters -- just return the output instead) is my preference, but that's not always possible (sometimes you really need to maintain mutable state, and sometimes that state consists of complex invariants), and even when it is possible (in my experience, it usually is), most libraries out there do not follow such a strict approach.
Fix for #839