Query json response using gjson #43

ZwodahS · 2017-02-08T15:19:49Z

the query syntax is using https://github.com/tidwall/gjson

If there is a need, we can create a new query syntax but that is not necessary at this moment.

I also took out the search code into search.go, I hope that is okay. This way, we can put all the searching code in one place.

Issue #40

Benaiah

This is a really good start - the gjson search works quite well, and the display of what wuzz thinks the content type is is clever. There's some things that I think need to be addressed before this can be merged, in addition to my notes on individual segments of the code:

You should be able to use textual search on any textual content, including JSON. IMO both gjson and xpath search should be optional. This touches on something I've been mulling on for a while, which is different kinds of search - personally, I'd like to add a regex search that shows the matching lines, instead of just the matching text. There needs to be some code organization and UI work to support this. IMO, we should wait for this to be decided on and implemented before adding any alternate forms of search, as I see this being very confusing for people who don't follow the development here on GitHub.
We need to figure out what the Request type should look like in the long-term. This is something I'm dealing with in one of my own branches (https://github.com/benaiah/wuzz/tree/json-highlighting), where I have similarly separated different kinds of request body.

I think the long-term solution to this should be to take this stuff out of Request, and leave that object unmodified by display code. I think we should have a separate struct to store modified versions of the request data - perhaps RequestDisplay. This is partially my fault - I started the trend of directly modifying Request with my JSON formatting patch early on, which mutated r.RawResponseBytes to format. I think that was a poor choice for long-term program structure.

In addition, I think @asciimoo should weigh in on the code organization. We'll definitely need to reorganize as the program grows, but it should probably be something thought-out and discussed for a while before we begin pulling stuff out of wuzz.go in an ad-hoc fashion. It impacts a lot more than just this search stuff - it requires that many or all of the existing PRs be modified to fit the new structure, it affects how the different parts of the application interoperate, and it has major effects on the git history.

Thanks so much for the PR! It's great work, and it's definitely a step towards a more flexible wuzz.

Benaiah · 2017-02-08T21:16:44Z

search.go

+		return SearchText(request, searchString)
+	}
+
+	if request.ContentType == "application/json" {


Matching the whole ContentType string does not work here - for instance, a Content-Type: application/json; charset=utf-8 results in [unknown content], and neither gjson nor textual search works in this case.

https://golang.org/pkg/mime/#ParseMediaType looks like the correct solution for determining the MIME value of a Content-Type header. We'll probably want to parse the header up-front and store it somewhere - perhaps Request.ContentType should be changed to use the following type:

type ContentType struct { MediaType string Params map[string]string }

This could then be populated by mime.ParseMediaType.

@asciimoo: thoughts?

I think there are many more things to do for this too. However, I personally like to have those fixes in a separate commit(s) instead, which is why I didn't want to include the mimetype detection in this commit.

That's fair, but in the meantime this should at least be changed to a strings.Contains(request.ContentType, "application/json") so it actually works consistently. The current behavior is really confusing if you're not developing wuzz.

Roger, will fix this and the one above with the "Index == -1"

Benaiah · 2017-02-08T21:18:02Z

search.go

+// returns title, display, error
+func Search(request *Request, searchString string) (string, []byte, error) {
+	isBinary := (strings.Index(request.ContentType, "text") == -1 &&
+		strings.Index(request.ContentType, "application") == -1)


Why do you use strings.Index(...) == -1 instead of !strings.Contains(...)? The latter would be clearer, IMO.

Oops, this is my bad. When I pull out the code, I didn't change the logic from the previous part.

ZwodahS · 2017-02-09T01:50:21Z

You should be able to use textual search on any textual content, including JSON. IMO both gjson and xpath search should be optional. This touches on something I've been mulling on for a while, which is different kinds of search - personally, I'd like to add a regex search that shows the matching lines, instead of just the matching text.

Agreed. I would like to add a key to toggle the type of search that you want to perform, with "auto" as the default.

There needs to be some code organization and UI work to support this. IMO, we should wait for this to be decided on and implemented before adding any alternate forms of search, as I see this being very confusing for people who don't follow the development here on GitHub.

I see. We should really decide this soon :P, seeing how many people are interested in helping.

We need to figure out what the Request type should look like in the long-term. This is something I'm dealing with in one of my own branches (https://github.com/benaiah/wuzz/tree/json-highlighting), where I have similarly separated different kinds of request body.

I think the long-term solution to this should be to take this stuff out of Request, and leave that object unmodified by display code. I think we should have a separate struct to store modified versions of the request data - perhaps RequestDisplay. This is partially my fault - I started the trend of directly modifying Request with my JSON formatting patch early on, which mutated r.RawResponseBytes to format. I think that was a poor choice for long-term program structure.

For this, I have moved them to a ParsedResponseBody which is a interface{} so that each type can use it store the parsed data. I believed that Xpath have something similar which is a *Node or some sort. I am also quite unsure If I should store this in app or request, which is why I went with the existing convention.

Personally I would store this as part of the display view.

In addition, I think @asciimoo should weigh in on the code organization. We'll definitely need to reorganize as the program grows, but it should probably be something thought-out and discussed for a while before we begin pulling stuff out of wuzz.go in an ad-hoc fashion. It impacts a lot more than just this search stuff - it requires that many or all of the existing PRs be modified to fit the new structure, it affects how the different parts of the application interoperate, and it has major effects on the git history.

Roger. So I guess I'll wait before continuing =)

Benaiah · 2017-02-09T01:55:06Z

@ZwodahS sounds good. @asciimoo has been active every day since the project started so far, so I'm not concerned that this will stall too long. He specifically requested that I don't merge PRs without him getting a chance to check them out, so it'd be waiting on him anyway.

For this, I have moved them to a ParsedResponseBody which is a interface{} so that each type can use it store the parsed data. I believed that Xpath have something similar which is a *Node or some sort. I am also quite unsure If I should store this in app or request, which is why I went with the existing convention.

Personally I would store this as part of the display view.

Agreed. When I have my highlighting branch ready I'll need to add another copy of the response body with highlighting, as you can't usefully search text that's full of terminal escape codes. I wouldn't want to put that in Request either (which is the current implementation in that branch - one of the reasons it's not ready for a PR).

Thanks again for the contribution, it's really very useful.

asciimoo · 2017-02-09T21:50:52Z

Agreed. I would like to add a key to toggle the type of search that you want to perform, with "auto" as the default.

This is exactly how I imagined this functionality =)

In addition, I think @asciimoo should weigh in on the code organization.

I don't like to over-engineer the problem. File based separation by logical blocks is enough at first imho. And we can create modules if we will have tests and more complex features. What do you think?

Agreed. When I have my highlighting branch ready I'll need to add another copy of the response body with highlighting, as you can't usefully search text that's full of terminal escape codes.

As I saw, gocui.View-s Buffer() function returns the text without the escape codes

Benaiah · 2017-02-09T22:17:57Z

I don't like to over-engineer the problem. File based separation by logical blocks is enough at first imho. And we can create modules if we will have tests and more complex features. What do you think?

Makes sense to me. I definitely don't think we're at the point we need submodules - the wuzz namespace isn't very crowded at the moment.

As I saw, gocui.View-s Buffer() function returns the text without the escape codes

Oh, awesome. Might not need to store an extra copy anyway.

I think the buffer/buffer display separation can probably wait for another PR with its own discussion, so I think that the code style/functionality problems I addressed are resolved (thanks for the quick work @ZwodahS!). All this is waiting on now is the search type toggling, as far as I can tell.

Benaiah · 2017-02-10T02:05:54Z

@ZwodahS you may already be aware of this, but when you rebase the PR onto the current master, you'll probably want to squash the [fix] commits so they don't end up in the project history (as they were reverts to things that were never in the history of asciimoo/wuzz master). If you're unsure of how to do this, there's a good guide here: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

ZwodahS · 2017-02-10T02:12:50Z

What kind of git workflow do you guys prefer ?

Do I squash the commits as I update it or do I squash all the commits when the PR is ready to be merged in ?
Do I rebase onto the the latest master or are you guys okay with merging from the old master that I branch from ?

Benaiah · 2017-02-10T02:27:19Z

@ZwodahS hmm, I've never actually discussed this with @asciimoo. This is the first PR where this has come up. I assumed we were planning on using a rebase/squash based workflow, as that's the norm, but if he prefers merges that's fine with me (it's his project after all). I think the [fix] commits should probably be removed from the history either way, but as for the overall merging strategy I'd have to defer.

As for question 1, I don't think it really matters whether you squash as you go or squash before the PR is good to go - it wouldn't affect the history of the main repo (that's the point of squashing).

ZwodahS · 2017-02-10T02:28:55Z

rebased and squashed.

I think this is what you want, if not let me know.

I kept them in 2 commits, because one refactored the code into a new file and the other added the query.

Benaiah · 2017-02-10T03:56:00Z

Yeah, the two commits should be fine IMO.

asciimoo · 2017-02-10T11:08:19Z

What kind of git workflow do you guys prefer ?

Usually, I don't have strict git workflow rules in my projects. Do we want here?
I prefer rebase+squash (except on master), but the quality of the code and the value of the contribution are much more important for me.

Benaiah · 2017-02-11T02:03:36Z

@asciimoo I'm in favor of rebasing, at least as the normal way to do it. It's the most common practice with Github-based open source projects, in my experience. My guess is that most people on Github are more used to using rebase to merge PRs. There might be situations where merge commits are more appropriate for history purposes or significantly easier to do, but I haven't seen any in this project. Rebase merging makes the history much easier to follow, IMO, so I'd definitely be in favor of it as a default, though I'm not dogmatic about it.

I don't think we should require that PRs get squashed into one commit, though I would like changes made during PR review to get squashed (like what @ZwodahS and I were discussing in this conversation), so we don't get changes in the history that were never actually part of the main repo. For instance, I think this PR is appropriate as two commits due to the separation of the search code into its own file, which is a pretty significant change that should be in the commit log, but I wouldn't want the commits that I addressed my review to be in the project history.

ZwodahS · 2017-02-11T04:20:56Z

Totally agreed with @Benaiah here on squashing into 1 commit if they aren't part of the original code and splitting them into different commits if they are of different nature.

The other question is whether if contributors should rebase to the new master or the one merging it.

ZwodahS · 2017-02-12T17:15:16Z

Hi, this is getting hard to rebase as more things are added to the search screen. In that case, I would suggest just closing this and refactor it later once things settle down and reimplement this later. Not really hard since we are just wrapped gjson here.

asciimoo · 2017-03-23T20:10:41Z

@ZwodahS sorry for the delayed response. I still think this is a great feature, but wuzz stabilized/changed a lot in the previous weeks. Could you please adapt the PR to the latest codebase?

ZwodahS · 2017-03-24T05:47:21Z

I don't think I will be able to find time to do this any time soon. I think someone should just take it and adapt it instead. Sorry about that.

asciimoo · 2017-03-24T22:10:38Z

@ZwodahS no problem, I'll do it. This PR is a good base, thank you for it.

asciimoo · 2017-10-15T21:02:15Z

added in 82d1ae6

ZwodahS changed the title ~~query json typed response using gjson~~ Query json response using gjson Feb 8, 2017

Benaiah suggested changes Feb 8, 2017

View reviewed changes

ZwodahS added 2 commits February 10, 2017 10:24

[refactor] Refactor search code out to another file

c894480

[enh] add query for json typed body, via gjson

704fbcc

ZwodahS force-pushed the master branch from 2849de7 to 704fbcc Compare February 10, 2017 02:27

asciimoo closed this Oct 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query json response using gjson #43

Query json response using gjson #43

ZwodahS commented Feb 8, 2017 •

edited

Loading

Benaiah left a comment •

edited

Loading

Benaiah Feb 8, 2017

Benaiah Feb 8, 2017

ZwodahS Feb 9, 2017

Benaiah Feb 9, 2017

ZwodahS Feb 9, 2017

Benaiah Feb 8, 2017

ZwodahS Feb 9, 2017

ZwodahS commented Feb 9, 2017

Benaiah commented Feb 9, 2017

asciimoo commented Feb 9, 2017

Benaiah commented Feb 9, 2017 •

edited

Loading

Benaiah commented Feb 10, 2017 •

edited

Loading

ZwodahS commented Feb 10, 2017

Benaiah commented Feb 10, 2017

ZwodahS commented Feb 10, 2017

Benaiah commented Feb 10, 2017

asciimoo commented Feb 10, 2017

Benaiah commented Feb 11, 2017

ZwodahS commented Feb 11, 2017

ZwodahS commented Feb 12, 2017

asciimoo commented Mar 23, 2017

ZwodahS commented Mar 24, 2017

asciimoo commented Mar 24, 2017

asciimoo commented Oct 15, 2017

Query json response using gjson #43

Query json response using gjson #43

Conversation

ZwodahS commented Feb 8, 2017 • edited Loading

Benaiah left a comment • edited Loading

Choose a reason for hiding this comment

Benaiah Feb 8, 2017

Choose a reason for hiding this comment

Benaiah Feb 8, 2017

Choose a reason for hiding this comment

ZwodahS Feb 9, 2017

Choose a reason for hiding this comment

Benaiah Feb 9, 2017

Choose a reason for hiding this comment

ZwodahS Feb 9, 2017

Choose a reason for hiding this comment

Benaiah Feb 8, 2017

Choose a reason for hiding this comment

ZwodahS Feb 9, 2017

Choose a reason for hiding this comment

ZwodahS commented Feb 9, 2017

Benaiah commented Feb 9, 2017

asciimoo commented Feb 9, 2017

Benaiah commented Feb 9, 2017 • edited Loading

Benaiah commented Feb 10, 2017 • edited Loading

ZwodahS commented Feb 10, 2017

Benaiah commented Feb 10, 2017

ZwodahS commented Feb 10, 2017

Benaiah commented Feb 10, 2017

asciimoo commented Feb 10, 2017

Benaiah commented Feb 11, 2017

ZwodahS commented Feb 11, 2017

ZwodahS commented Feb 12, 2017

asciimoo commented Mar 23, 2017

ZwodahS commented Mar 24, 2017

asciimoo commented Mar 24, 2017

asciimoo commented Oct 15, 2017

ZwodahS commented Feb 8, 2017 •

edited

Loading

Benaiah left a comment •

edited

Loading

Benaiah commented Feb 9, 2017 •

edited

Loading

Benaiah commented Feb 10, 2017 •

edited

Loading