Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions core/src/main/java/feign/querymap/FieldQueryMapEncoder.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import feign.codec.EncodeException;
import java.lang.reflect.Field;
import java.util.*;
import java.util.stream.Collectors;

/**
* the query map will be generated using member variable names as query parameter names.
Expand Down Expand Up @@ -66,14 +67,11 @@ private ObjectParamMetadata(List<Field> objectFields) {
}

private static ObjectParamMetadata parseObjectType(Class<?> type) {
List<Field> fields = new ArrayList<Field>();
for (Field field : type.getDeclaredFields()) {
if (!field.isAccessible()) {
field.setAccessible(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

@rage-shadowman we already do it

Copy link
Contributor

@rage-shadowman rage-shadowman Nov 13, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

But setAccessible is void or am I missing something obvious?

Copy link
Contributor

@rage-shadowman rage-shadowman Nov 13, 2018

Choose a reason for hiding this comment

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

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.

}
fields.add(field);
}
return new ObjectParamMetadata(fields);
return new ObjectParamMetadata(
Arrays.stream(type.getDeclaredFields())
.filter(field -> !field.isSynthetic())
.peek(field -> field.setAccessible(true))
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, this violates the non-interference contract, but I don't see any actual issues with it.

.collect(Collectors.toList()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class PropertyQueryMapEncoderTest {
/**
* Test for {@link BeanQueryMapEncoder}
*/
public class BeanQueryMapEncoderTest {

@Rule
public final ExpectedException thrown = ExpectedException.none();
Expand Down
67 changes: 67 additions & 0 deletions core/src/test/java/feign/querymap/FieldQueryMapEncoderTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* Copyright 2012-2018 The Feign Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/
package feign.querymap;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import java.util.HashMap;
import java.util.Map;
import feign.QueryMapEncoder;

/**
* Test for {@link FieldQueryMapEncoder}
*/
public class FieldQueryMapEncoderTest {

@Rule
public final ExpectedException thrown = ExpectedException.none();

private final QueryMapEncoder encoder = new FieldQueryMapEncoder();

@Test
public void testDefaultEncoder_normalClassWithValues() {
final Map<String, Object> expected = new HashMap<>();
expected.put("foo", "fooz");
expected.put("bar", "barz");
final NormalObject normalObject = new NormalObject("fooz", "barz");

final Map<String, Object> encodedMap = encoder.encode(normalObject);

assertEquals("Unexpected encoded query map", expected, encodedMap);
}

@Test
public void testDefaultEncoder_normalClassWithOutValues() {
final NormalObject normalObject = new NormalObject(null, null);

final Map<String, Object> encodedMap = encoder.encode(normalObject);

assertTrue("Non-empty map generated from null getter: " + encodedMap, encodedMap.isEmpty());
}

class NormalObject {

private NormalObject(String foo, String bar) {
this.foo = foo;
this.bar = bar;
}

private final String foo;
private final String bar;
}

}