Added support for custom POJO query param encoding #667

rage-shadowman · 2018-03-23T22:31:30Z

Added support for custom POJO parsing of optional query parameters.

Updated the existing QueryMap annotation to be used on custom POJO objects as well as Maps:

@RequestLine("GET /")
V find(@QueryMap(encoded = BOOLEAN_VALUE) CustomObject customObject);

The CustomObject class must have fields that are named as they shall appear in the query parameter map. The object will be used to generate a Map<String,Object> through reflection based on all local fields that will then be handled in the same way as a regular QueryMap annotated Map object.

You cannot have more than 1 QueryMap annotated parameter (you cannot even have 1 that is a Map and another that is a non-map POJO). There is no way to specify that some fields are encoded, while others are not, and there is no way to specify a custom Expander for any fields.

Fixes #661 (may also fix #636 and #620 with some local modifications).

[EDIT: Updated per changes in later commits based on pull review comments]

rage-shadowman

Self-check commentary

rage-shadowman · 2018-03-24T04:36:43Z

core/src/main/java/feign/Contract.java

@@ -282,11 +282,26 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data, Annotation[
          checkState(data.headerMapIndex() == null, "HeaderMap annotation was present on multiple parameters.");
          data.headerMapIndex(paramIndex);
          isHttpAnnotation = true;
+        } else {
+          CustomParam customParam = findCustomParam(annotation);


NOTE: This checks for extensions of @CustomParam (as explained in the initial pull request comment) as well as the simple annotation instanceof CustomParam, but it does not search recursively, as I don't believe it makes sense to extend an extension. This extension checking is the only thing that my untrained eye can see in this pull request that might be "iffy" (I like it, but can obviously pull it out if there are objections).

Of course, please let me know if this (or something I didn't see) stands out as needing changes. I would love to see some sort of custom param encoder (such as this) put into feign (and from some of the other open issues/pull requests, I can see that I'm not alone).

saintf · 2018-03-28T10:20:42Z

hi @rage-shadowman -
I took a peek and tried to understand the problem we're trying to solve. As I see it, what all of the above have roughly in common is that they're trying to pass complex objects as parameters and have them be then placed in the request as either query parameters, path parameters or headers.

That, however, is difficult without some code like above. However, my feeling is that even now the code is too difficult. Feign has always (at least to me) been about simplicity and making things declarative (instead of having to write complex code to achieve something). Most of the internals of feign are completely unknown to the common user (i.e. passing the template/etc seems wrong to me).

Today Feign already allows you to put values into headers dynamically, as well as parameters and query strings. It even allows you to pass a map to be a query param (as long as you use @QueryParam)

    interface GitHub {
        @RequestLine("GET /repos/{owner}/{repo}/contributors")
        List<Contributor> contributors(@Param("owner") String owner, @Param("repo") String repo);

        @RequestLine("GET /users/{username}/repos?sort={sort}&filter={filterPattern}")
        @Headers("X-Ping: {token}")
        List<Repo> repos(@Param("username") String owner, @Param("sort") String sort,@Param("token") String token,@Param("filterPattern") String pattern);

        @RequestLine("GET /find")
        V find(@QueryMap Map<String, Object> queryMap);
    }

That said - I think perhaps a more natural way to pass this without having to expand it is likely where we add value - so what if we decided to have something like this:

    class RepoDetails {
        String owner;
        String repoName;
    }

    class QueryObject {
        String sort;
        String filter;
    }

    interface GitHub {
        @RequestLine("GET /repos/{repo.owner}/{repo.repoName}/contributors")
        List<Contributor> contributors(@Param("repo") RepoDetails repoDetails);

        //this should yield GET /users/{username}/repos?sort={query.sort}&filter={query.filter}
        @RequestLine("GET /users/{username}/repos")
        @Headers("X-Ping: {token}")
        List<Repo> repos(@Param("username") String owner, @QueryMap QueryObject query);
    }

Lastly, I think mixing parameters (query/path) with headers sounds slightly dangerous and likely something to be avoided. That said - if you have the usecase perhaps the library supports it enough to let you do it if you must. I would then do something of the sort:

    class RepoDetails {
        String owner;
        String repoName;
        String accessToken;
    }

    interface GitHub {
        @RequestLine("GET /repos/{repo.owner}/{repo.repoName}/contributors")
        @Headers("X-Ping: {repo.accesToken}")
        List<Contributor> contributors(@Param("repo") RepoDetails repoDetails);
    }

This way everything remains declarative - with very little code to write from any of the users.

What do you think?

rage-shadowman · 2018-03-28T21:12:15Z

Regarding template access, if you write an Encoder or Decoder you get access to both the parameter object and the template (but only for un-annotated "body" parameters, of which you can only have 1) and if you write a RequestInterceptor you get access to the template (but no parameter objects). So you already have to know something about the template in order to use Feign's advanced functionality.

However, I agree that simplicity is preferred. So...

I could rename CustomParam to ObjectParam and include some HeaderParam(header = "X-Header-Name", expander = OptionalExpander.class), QueryParam(name = "param-name", expander = OptionalExpander.class, encoded = DEFAULTS_TO_FALSE), and BodyParam(encoder = OptionalEncoder.class) with a reflection-based ObjectParamEncoder class. Also allowing a HeaderMap and QueryMap style annotation as well. All annotations would work on either member variables (private access is fine) or 0-arg (getter style) methods.

Would that answer your concerns?

As far as the "{parent.child}" format, I could see allowing an optional name parameter on the ObjectParam annotation that would prefix all child parameters (parentName + "." + childName).

I don't like the idea of parsing field or method names to determine the parameter inclusion characteristics, so I'd stick with annotations. I also like the idea of this object parameter not necessarily knowing all of the possible params ahead of time (allowing for "map" style parameters).

kdavisk6 · 2018-03-29T14:11:39Z

My gut reaction to this is that addition of the encoder option in the annotation feels awkward and may be too complicated as @saintf has mentioned. Also, adding an additional annotation where the general purpose of QueryMap feels like this should work here, makes we wonder, could we update @QueryMap to support Map and Object values?

In the case of an Object, we can use a technique similar to what you and @DanielYWoo have suggested to expand the object. Users can then do something like the following

public class SearchDetails {
	public String term;
	public List<String> filters;
	public String sortOn;
	public String sortDirection
}

interface Search {
        // the result here would be `/api/search?term={term}&filters={filter[0]}&filters={filter[1]}...&sortOn={sortOn}&sortDirection={sortDirection}
	@RequestLine("GET /api/search")
	List<SearchResults> search(@QueryMap SearchDetails searchDetails);
}

To me, this fits more within the spirit of the change you are requesting without making significant modifications to the current usage patterns. Thoughts?

rage-shadowman · 2018-03-29T21:24:26Z

@kdavisk6 I like that. But I'd suggest adding a new QueryObject annotation instead (just so people don't get confused seeing @QueryMap RandomNonMapPojo and thinking it's a bug).

And I'd still suggest an annotation like Param for those objects so that we could change the param name to be different than the object field name (though I can also see an argument that it be an optional annotation that just overrides the default behavior).

I would also would need a HeaderObject version as well since RequestInterceptor doesn't give access to the parameter objects (see #601).

kdavisk6 · 2018-03-29T21:34:35Z

As long as we properly document the use, I think we can resolve any confusion that may appear. With regards to your additional desires, @rage-shadowman, I think we should strive for simplicity in this first attempt, and focus primarily on @QueryMap and expanding parameters. Expanding the scope of what can be done within a RequestInterceptor feels like a change we should work on separately.

saintf · 2018-03-30T08:58:43Z

So do we then agree that in this change (at least for first round) we focus on expanding @QueryMap to take on objects and expanding the parameters (not just taking a map)?

I'd vote for that as an initial change - then look at the next set of desires as separate changes (equally happy to get my hands dirty and do some coding if you'd like some help @rage-shadowman)

rage-shadowman · 2018-03-31T04:41:43Z

@saintf I think that sounds good. I'd prefer to call it something else, but if that's the consensus, I'll try to get that working and submit the changes. I'll still need to hack in an authorization interceptor for my needs, but I'll keep that code separate. This should be a great start for what we need.

I'd also like to make it work only on the object's fields or methods annotated with @Param, rather than parsing the getter names like a bean or assuming all fields are included as named. This way we can also specify whether or not a parameter is encoded and if a custom Expander is needed. Is there any consensus on this?

kdavisk6 · 2018-04-02T13:21:22Z

I think that adding additional annotations to fine tune the encoding would be a welcome feature, however; let's start small. Referring back to The Rule of Three,

a reusable component should be tried out in three different applications before it will be sufficiently general to accept into a reuse library.

we should focus on adding basic support for Objects via @QueryMap and see how adoption goes. If it goes well, and with this particular ask I don't see that as being an issue, we can expand on it. Sound good?

rage-shadowman · 2018-04-03T01:14:49Z

core/src/main/java/feign/ObjectParamMetadata.java

+class ObjectParamMetadata {
+
+  private final static ConcurrentMap<Class<?>, ObjectParamMetadata> classToMetadata =
+      new ConcurrentHashMap<Class<?>, ObjectParamMetadata>();


I'm not sure if this cache is a case of premature optimization or not.

...or if it belongs in MethodMetadata instead (like indexToExpander).

I think I start getting the point of this class. Is it basically a QueryObjectParamMapper or so? as in - it's job is to, for a single object type, generate a Map<String,Object>?

If so, I think I get where you're headed and would suggest the following changes:

Rename it to something closer to the description above. I like QueryObjectParamMapper, but if you think of a better one, thumbs up.

The caching is likely not a premature optimisation (based on how many other places we try to figure out a class and then cache its metadata to not have it done again) - but is likely in the wrong place (why would it be done in this object?). Perhaps moving it to the place where it gets called from (ReflectiveFeign?) is a more appropriate fit? It also doesn't have to be static there - which is usually a bit of a code-smell.

likely rename toQueryMap to something more like mapObject(object) or convertToQueryMap - the mapper isn't becoming a query map itself - it's converting an object into one - so make it that descriptive

likely rename getMetadata to generateMapper (if you end up calling this a mapper) - and you can likely then make it the single-entry point on startup that I mentioned.

Thoughts?

oh! and given the somewhat more complex nature - add some unit tests specifically to it - to make sure you capture intended behaviour and can replicate error cases easier. (test both correct behaviour and expected error cases)

I read some more on your code. I agree on the right place being next to indexToExpander. If you want I can fork off your repo, give it a try, and then create PR into your repo for those changes so you can see what I mean/accept if you like.

I named it based on the other metadata class, because that's what it stores. But you are right, what it does is turn an object into a map, so I'll rename it.

Some possibilities: QueryObjectParamMapper, ObjectQueryParamMapper, QueryMapBuilder, or keep the cache internal and call it QueryMapFactory (either keep it static, or make it an interface default implementation that you can add as a Feign.Builder option and pass it into the ReflectiveFeign constructor)? It actually is building a Map of query params (so it is a QueryMapBuilder or "factory").

How does that sound?

@saintf I don't mind working on this. It's a good learning experience for me. But if you want to write some code on it, I'm happy to do look at that too.

rage-shadowman · 2018-04-03T01:18:20Z

I believe I have followed the prior input. Please look again and let me know what you think. Did I miss or misunderstand anything?

If this is good, then I can start over from a clean branch so this is all done in 1 commit if you like (to make the commit history cleaner).

… used

spencergibb

No need for a new pull request. Commits can be squashed.

rage-shadowman · 2018-04-03T23:13:54Z

Thanks for the comments. I do think this simplifies things and that makes it better. A Builder pattern for your custom POJO objects should allow this to work for anything that just needs to modify query params (headers are another matter, but I think actually allowing RequestInterceptors access to the method arguments would be the better solution there).

rage-shadowman · 2018-04-05T23:09:17Z

So, where does this stand? Is it waiting for votes? Waiting for @adriancole or another maintainer to review?

velo · 2018-04-06T00:12:50Z

Yes, I need to take a look on this, just busy this week =)

saintf

Taking a nice shape. I think it needs a little restructuring/some changes on naming, but definitely headed in the right direction to me (almost there).

Nice one @rage-shadowman

saintf · 2018-04-07T12:49:05Z

core/src/main/java/feign/ReflectiveFeign.java

@@ -212,7 +211,10 @@ public RequestTemplate create(Object[] argv) {
      if (metadata.queryMapIndex() != null) {
        // add query map parameters after initial resolve so that they take
        // precedence over any predefined values
-        template = addQueryMapQueryParameters((Map<String, Object>) argv[metadata.queryMapIndex()], template);
+        boolean encoded = metadata.queryMapEncoded();


correct me if wrong - but encoded doesn't get used anywhere.

Also - just a thought on style - but you can remove the value parameter without loosing too much readability by doing the following:

Map<String, Object> queryMap = toQueryMap(argv[metadata.queryMapIndex()]); template = addQueryMapQueryParameters(queryMap, template);

saintf · 2018-04-07T12:55:33Z

core/src/main/java/feign/ReflectiveFeign.java

+        return (Map<String, Object>)value;
+      }
+      try {
+        return ObjectParamMetadata.getMetadata(value.getClass()).toQueryMap(value);


ObjectParamMetadata seems like some helper of some sort... but it forces callers to do things in a certain order (you must first call getMetadata(object) to then ask it to turn into a query map. There's a lot of separation of concerns problems here. If the objective of ObjectParamMetadata is to get metadata, then it likely should be this class that generates queryParams. If the ObjectParamMetadata is a helper, likely it should return the queryParams directly (without forcing users to have to know of internal implementation/how-to cal/which order).

Finally - static methods are usually tough to test. In this case, I guess they do get covered by the wider tests you've written - but there aren't any specific tests for ObjectParamMetadata helper.

You're right, it should either have the cache external with the current factory method, or make toQueryMap the only visible static method.

As you mentioned elsewhere, it should also be renamed as it has no getters for the metadata, but uses them itself to build a map from an object.

The evolution of this class got a bit messy. It needs some cleanup. My response on the other bullet-pointed comment has some questions that should help me to know how to clean it up.

Personally, I'm leaning towards it being a non-static (with cache internal) default implementation of a QueryMapFactory interface that can be passed into the Feign.Builder and added to ReflectiveFeign alongside the InvocationHandlerFactory.

Once its behavior is solidified, tests should be added.

saintf · 2018-04-07T13:07:25Z

core/src/main/java/feign/Contract.java

+      checkMapKeys(name, genericType);
+    }
+
+    private static void checkMapKeys(String name, Type genericType) {


this looks like a good add - but I didn't see a test covering it in the case that it isn't a string (I assume tests already cover the case where it is)

This wasn't an addition, it was a DRY split. I suppose I could revert the checkMapString method and just leave the Map instance check even though I just checked that. The test is in DefaultContractTest.queryMapKeysMustBeStrings.

saintf · 2018-04-07T13:18:25Z

README.md

+    this.number = number;
+  }
+}
+```


nice docs.

We should discuss with the wider audience if we want all member variables (including non-exposed private ones), or if we want those that are exposed in some way (public or have a getter), and if we handle transient ones or not. It's worth asking as I see below in the code that we're changing access modifiers on the fly (making accessible if not) - something that may not be something we do out-of-the box or so...

In another PR of a similar nature, I made the suggestion that we should support objects via the Java Beans api. That should allow for the widest range of support and interoperability. However straight reflection is ok too from my perspective.

I'd still argue in favor of annotating the methods/fields that you want to use with something like @Param. The only reason to use all fields, was for simplicity (I guess additional annotations weren't simple enough). Personally, I'm less a fan of parsing all getter method names (javabeans style) than I am of reading all fields. But if it'll get this stuff merged...

@rage-shadowman let's get more thoughts on it from a few people. It may be that the concensus is this works just fine and we like it (and then just leave it alone). It may also be that we go Java Beans style or something else. let's see what others think before taking any drastic decision.

If the map builder has an interface that can be passed into the feign builder, then I could even write my own in my local client that would use my annotations without complicating anything internal to feign. We could then have an EJB implementation that parses getters as a separate possibility.

I agree that a Serializer would fit nicely, so as long as we have a sensible default Serializer.

I created a QueryMapEncoder that can be specified via the Feign.Builder. I think it answers this concern and cleans up the naming issues.

I haven't pushed it into this pull request as I don't want to jump the gun on this. You can check that out in https://github.com/rage-shadowman/feign/tree/feature/shadowman/query-param-encoder

Should I push that into this pull request?

Is there any agreement here? Shall I make the QueryMap encoder something to be added to the Feign.builder() as in the aforementioned branch (this one gets my vote)? Should I add it as a parameter to QueryMap requiring a 0-arg public constructor (similar to Param.Expander)? Or should I just clean up the naming and packaging of what is here and call it done?

@rage-shadowman I'd add your encoder to the PR and suggest that it be used by default, without any additional customization to start.

saintf · 2018-04-07T13:34:54Z

core/src/main/java/feign/ObjectParamMetadata.java

+class ObjectParamMetadata {
+
+  private final static ConcurrentMap<Class<?>, ObjectParamMetadata> classToMetadata =
+      new ConcurrentHashMap<Class<?>, ObjectParamMetadata>();


I think I start getting the point of this class. Is it basically a QueryObjectParamMapper or so? as in - it's job is to, for a single object type, generate a Map<String,Object>?

If so, I think I get where you're headed and would suggest the following changes:

Rename it to something closer to the description above. I like QueryObjectParamMapper, but if you think of a better one, thumbs up.

The caching is likely not a premature optimisation (based on how many other places we try to figure out a class and then cache its metadata to not have it done again) - but is likely in the wrong place (why would it be done in this object?). Perhaps moving it to the place where it gets called from (ReflectiveFeign?) is a more appropriate fit? It also doesn't have to be static there - which is usually a bit of a code-smell.

likely rename toQueryMap to something more like mapObject(object) or convertToQueryMap - the mapper isn't becoming a query map itself - it's converting an object into one - so make it that descriptive

likely rename getMetadata to generateMapper (if you end up calling this a mapper) - and you can likely then make it the single-entry point on startup that I mentioned.

Thoughts?

saintf · 2018-04-07T13:36:16Z

core/src/main/java/feign/ObjectParamMetadata.java

+class ObjectParamMetadata {
+
+  private final static ConcurrentMap<Class<?>, ObjectParamMetadata> classToMetadata =
+      new ConcurrentHashMap<Class<?>, ObjectParamMetadata>();


oh! and given the somewhat more complex nature - add some unit tests specifically to it - to make sure you capture intended behaviour and can replicate error cases easier. (test both correct behaviour and expected error cases)

saintf · 2018-04-07T13:37:22Z

core/src/test/java/feign/FeignTest.java

@@ -384,6 +383,48 @@ public void queryMapValueStartingWithBrace() throws Exception {
        .hasPath("/?%7Bname=%7Balice");
  }

+  @Test


I like 👍 😄 . Nice, clean, easily readable test cases.

saintf · 2018-04-07T14:16:31Z

core/src/main/java/feign/ObjectParamMetadata.java

+class ObjectParamMetadata {
+
+  private final static ConcurrentMap<Class<?>, ObjectParamMetadata> classToMetadata =
+      new ConcurrentHashMap<Class<?>, ObjectParamMetadata>();


I read some more on your code. I agree on the right place being next to indexToExpander. If you want I can fork off your repo, give it a try, and then create PR into your repo for those changes so you can see what I mean/accept if you like.

velo

LGTM, but I think we need to at least discuss this a bit further
https://github.com/OpenFeign/feign/pull/667/files#r179939686

…ault implementation provided)

velo · 2018-04-22T08:53:20Z

I was about to merge this, but there is one conflict. Ping me once fixed and I will hit the button.

…n/custom-param-encoding

rage-shadowman · 2018-04-23T17:34:58Z

Fixed the conflicts. Though I am unable to test it as your master fails tests for me right now (and thus so does my branch now that I've merged your master back into it).

Looks like it passed tests in travis-ci though.

velo · 2018-04-23T20:52:27Z

Fixed the conflicts. Though I am unable to test it as your master fails tests for me right now (and thus so does my branch now that I've merged your master back into it).

Can you detail the failures and your system?
May be is a OS related issue and may be relevant to fix it.

rage-shadowman · 2018-04-23T21:15:28Z

It had to do with redirects and retries (the retry tests failed because of a permanent redirection response). But now I'm at work, and it's working, so I'm guessing it must have been a DNS/router issue with my home wifi.

Also, the tests were failing for me from OpenFeign/master, not just from my branch.

* Added support for custom param encoding * Added ability to inherit @CustomParam annotation * Updated class cast style to match rest of code * Updated to use QueryMap for custom pojo query parameters * Clarification in README of QueryMap POJO usage * Removed unused line * Updated custom POJO QueryMap test to prove that private fields can be used * Removed no-longer-valid test endpoint * Renamed tests to more accurately reflect their contents * More test cleanup * Modified QueryMap POJO encoding to use specified QueryMapEncoder (default implementation provided) * Corrected typo in README.md * Fixed merge conflict and typo in test name

Added support for custom param encoding

f4be32f

rage-shadowman mentioned this pull request Mar 23, 2018

Support Object Query with GET request #620

Closed

Added ability to inherit @CustomParam annotation

6b8ac82

rage-shadowman mentioned this pull request Mar 24, 2018

Provide RequestTemplate.getParams() to access template variables (@Param) within an interceptor #601

Open

rage-shadowman commented Mar 24, 2018

View reviewed changes

Updated class cast style to match rest of code

0eea796

rage-shadowman mentioned this pull request Mar 28, 2018

Add the ability to pass request parameters as object #636

Closed

Ralph Jennings added 2 commits April 2, 2018 18:10

Updated to use QueryMap for custom pojo query parameters

186f66c

Clarification in README of QueryMap POJO usage

4867c11

rage-shadowman commented Apr 3, 2018

View reviewed changes

Removed unused line

54a0e9e

Ralph Jennings added 4 commits April 2, 2018 18:29

Updated custom POJO QueryMap test to prove that private fields can be…

dde5e10

… used

Removed no-longer-valid test endpoint

7d72d22

Renamed tests to more accurately reflect their contents

a44c472

More test cleanup

336f6bf

spencergibb reviewed Apr 3, 2018

View reviewed changes

rage-shadowman changed the title ~~Added support for custom param encoding~~ Added support for custom POJO query param encoding Apr 4, 2018

saintf suggested changes Apr 7, 2018

View reviewed changes

velo approved these changes Apr 8, 2018

View reviewed changes

Ralph Jennings added 2 commits April 19, 2018 14:58

Modified QueryMap POJO encoding to use specified QueryMapEncoder (def…

1c3f822

…ault implementation provided)

Corrected typo in README.md

eb8faf9

Ralph Jennings added 2 commits April 23, 2018 10:11

Fixed merge conflict and typo in test name

818deb5

Merge remote-tracking branch 'openfeign/master' into feature/shadowma…

7fdafd0

…n/custom-param-encoding

velo merged commit 5e2fdee into OpenFeign:master Apr 23, 2018

MikeSafonov mentioned this pull request Jun 12, 2018

Up Feign version and using DTO in GET request as QueryMap spring-cloud/spring-cloud-openfeign#34

Closed

kdavisk6 mentioned this pull request Jul 26, 2018

How to pass request parameters as object ? #520

Closed

Menighin mentioned this pull request Aug 13, 2021

@QueryMap not resolving to query parameters #1484

Closed

Added support for custom POJO query param encoding #667

Added support for custom POJO query param encoding #667

Conversation

rage-shadowman commented Mar 23, 2018 • edited Loading

rage-shadowman left a comment

Choose a reason for hiding this comment

rage-shadowman Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

saintf commented Mar 28, 2018 • edited Loading

rage-shadowman commented Mar 28, 2018 • edited Loading

kdavisk6 commented Mar 29, 2018

rage-shadowman commented Mar 29, 2018

kdavisk6 commented Mar 29, 2018

saintf commented Mar 30, 2018

rage-shadowman commented Mar 31, 2018

kdavisk6 commented Apr 2, 2018

Choose a reason for hiding this comment

rage-shadowman Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rage-shadowman commented Apr 3, 2018 • edited Loading

spencergibb left a comment

Choose a reason for hiding this comment

rage-shadowman commented Apr 3, 2018 • edited Loading

rage-shadowman commented Apr 5, 2018

velo commented Apr 6, 2018

saintf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rage-shadowman Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdavisk6 Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rage-shadowman Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

velo left a comment

Choose a reason for hiding this comment

velo commented Apr 22, 2018

rage-shadowman commented Apr 23, 2018 • edited Loading

velo commented Apr 23, 2018

rage-shadowman commented Apr 23, 2018 • edited Loading

rage-shadowman commented Mar 23, 2018 •

edited

Loading

rage-shadowman Mar 24, 2018 •

edited

Loading

saintf commented Mar 28, 2018 •

edited

Loading

rage-shadowman commented Mar 28, 2018 •

edited

Loading

rage-shadowman Apr 3, 2018 •

edited

Loading

rage-shadowman commented Apr 3, 2018 •

edited

Loading

rage-shadowman commented Apr 3, 2018 •

edited

Loading

rage-shadowman Apr 7, 2018 •

edited

Loading

kdavisk6 Apr 7, 2018 •

edited

Loading

rage-shadowman Apr 8, 2018 •

edited

Loading

rage-shadowman commented Apr 23, 2018 •

edited

Loading

rage-shadowman commented Apr 23, 2018 •

edited

Loading