fix paging #192

tomerweller · 2017-12-05T23:23:18Z

apply query filters on paging links. closes #148

also:

modifies the Page struct to include a full url, rather than separate base url, path and query params.
introduces a support/url package and a URL struct for convenient url modifications.

nikhilsaraf

looks solid!
added some comments/questions, nothing major.

nikhilsaraf · 2017-12-08T18:58:39Z

support/url/main.go

+	if err != nil {
+		return
+	}
+	return URL(*gu), nil


we should not be returning specific values if this function has named return values.

nikhilsaraf · 2017-12-08T19:00:07Z

support/url/main.go

+
+// String encodes all URL segments to a fully qualified URL string
+func (u URL) String() string {
+	gu := gUrl.URL(u)


can we not just call u.String()? -- i.e. maybe just leave off this method altogether?

if not -- return gUrl.URL(u).String(), doesn't need the passthrough var gu

In order to do that, you'd need to convert the sURL into a gURL every-time you want to call it...

Adding a passthrough gives the following error:

# github.com/stellar/go/support/url ../../../support/url/main.go:24:21: cannot call pointer method on url.URL(*u) ../../../support/url/main.go:24:21: cannot take the address of url.URL(*u)

Maybe @nullstyle or one of the golang experts can weigh in on this.

nikhilsaraf · 2017-12-08T19:01:17Z

support/url/main.go

+	q := gu.Query()
+	q.Del(key)
+	q.Add(key, val)
+	gu.RawQuery = q.Encode()


why do we need to call q.Encode() here, will it still work if we just delete this line?
it seems hacky to override gu.RawQuery, almost like we're setting values on gu twice.

RawQuery is a string. The Query() function decodes current RawQuery string and returns a Values struct. I don't see another way to do this.

what I'm suggesting is -- is gu.RawQuery = q.Encode() a nop here?

No, unfortunately this is how the URL struct is built

nikhilsaraf · 2017-12-08T19:02:29Z

services/horizon/internal/actions_trade.go

-	newUrl := action.BaseURL()              // preserve scheme and host for the new url links
-	newUrl.RawQuery = action.R.URL.RawQuery //preserve query parameters
-	newUrl.Path = action.Path()             //preserve path
+	newUrl := action.FullURL()              // preserve scheme and host for the new url links


👍 for a cleaner code pattern

nikhilsaraf · 2017-12-08T19:03:58Z

services/horizon/internal/actions_assets_test.go

-			ht.Assert.Equal(("http://localhost" + kase.wantSelf), links.Self.Href)
-			ht.Assert.Equal(("http://localhost" + kase.wantPrevious), links.Prev.Href)
-			ht.Assert.Equal(("http://localhost" + kase.wantNext), links.Next.Href)
+			ht.Assert.EqualUrlStrings("http://localhost" + kase.wantSelf, links.Self.Href)


I don't think you have gofmt enabled on save? --that would have removed the " " around the + here, which is why I had the ()

nikhilsaraf · 2017-12-08T19:05:00Z

services/horizon/internal/actions_assets.go

@@ -88,5 +87,5 @@ func (action *AssetsAction) loadPage() {
 	if action.AssetIssuer != "" {
 		linkParams.Set("asset_issuer", action.AssetIssuer)
 	}
-	action.Page.PopulateLinksWithParams(linkParams)


you should also remove the linkParams var here since it's no longer used --your compiler should have issued warning about this?

nikhilsaraf · 2017-12-08T19:07:45Z

services/horizon/internal/action.go

+	return result
+}
+
+// BaseURL returns the base url for this request, defined as a url containing
 // the Host and Scheme portions of the request uri.
 func (action *Action) BaseURL() *url.URL {


if FullURL is the only consumer of BaseURL now then we should lowercase this method name.

nikhilsaraf · 2017-12-08T19:10:34Z

services/horizon/internal/render/hal/page.go

@@ -39,37 +41,40 @@ type Links struct {
 type Page struct {
 	Links Links `json:"_links"`
 	BasePage
-	BasePath string `json:"-"`
+	//BasePath string `json:"-"`


delete instead of commenting out

nikhilsaraf · 2017-12-08T19:12:37Z

services/horizon/internal/render/hal/page.go

 	rec := p.Embedded.Records

+	//verify paging params
+	selfUrl := sUrl.URL(*p.FullURL).


don't think you need the * here.

nikhilsaraf

looks like there's still some comments you may be working through, let me know when to re-review.

nikhilsaraf · 2017-12-11T19:20:43Z

support/url/main.go

+	q := gu.Query()
+	q.Del(key)
+	q.Add(key, val)
+	gu.RawQuery = q.Encode()


what I'm suggesting is -- is gu.RawQuery = q.Encode() a nop here?

nikhilsaraf

LGTM!

nikhilsaraf · 2017-12-12T19:02:13Z

See the one travis failure before merging.

tomerweller · 2017-12-12T22:17:11Z

@nullstyle I went ahead and merged this. When your table clears you can take a look to see if you have some feedback from an idiomatic perspective. not urgent at all.

fix error introduces in #192

nullstyle · 2017-12-13T00:45:25Z

services/horizon/internal/render/hal/page.go

@@ -1,13 +1,15 @@
 package hal

 import (
+	sUrl "github.com/stellar/go/support/url"


package names should always be lower cased: https://blog.golang.org/package-names

We should do the same with our aliases, IMO

nullstyle · 2017-12-13T00:48:36Z

support/url/main.go

+	gu := gUrl.URL(u)
+	q := gu.Query()
+	q.Del(key)
+	q.Add(key, val)


instead of using Del then Add, just use Set.

https://golang.org/src/net/url/url.go?s=22624:22662#L779

nullstyle

.

nullstyle · 2017-12-13T01:09:14Z

support/url/main.go

+)
+
+// URL wraps around the native golang URL struct to allow for custom methods
+type URL gUrl.URL


I'm not really happy with this whole type. Go isn't an object-oriented language and this smells too much like you're trying to derive a subtype of URL so you can override some methods. Instead of pretending this type is a URL lets call this type a URLBuilder and lets have it wrap a url.URL, not alias it.

See https://github.com/lann/builder for a pretty well worn implementation of the fluent builder pattern you are proposing here.

tomerweller added 3 commits December 5, 2017 15:14

apply query filters on paging links. closes stellar#148

bd09585

improve documentation

1bb6f9b

replace page baseurl with fullurl

6da016f

tomerweller added the horizon label Dec 5, 2017

tomerweller requested review from nullstyle, bartekn and nikhilsaraf December 5, 2017 23:23

tomerweller added the in progress label Dec 5, 2017

nikhilsaraf reviewed Dec 8, 2017

View reviewed changes

fixups

1481848

nikhilsaraf reviewed Dec 11, 2017

View reviewed changes

typo

8e0e952

nikhilsaraf approved these changes Dec 12, 2017

View reviewed changes

tomerweller merged commit cb5972a into stellar:master Dec 12, 2017

tomerweller removed the in progress label Dec 12, 2017

tomerweller added a commit to tomerweller/go that referenced this pull request Dec 12, 2017

fix error introduces in stellar#192

9bd9b67

jedmccaleb pushed a commit that referenced this pull request Dec 12, 2017

Merge pull request #212 from tomerweller/trades_fixup

62e0333

fix error introduces in #192

nullstyle reviewed Dec 13, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix paging #192

fix paging #192

tomerweller commented Dec 5, 2017

nikhilsaraf left a comment

nikhilsaraf Dec 8, 2017

nikhilsaraf Dec 8, 2017

tomerweller Dec 9, 2017 •

edited

Loading

tomerweller Dec 9, 2017

nikhilsaraf Dec 8, 2017

tomerweller Dec 9, 2017

nikhilsaraf Dec 11, 2017

tomerweller Dec 11, 2017

nikhilsaraf Dec 8, 2017

nikhilsaraf Dec 8, 2017

nikhilsaraf Dec 8, 2017

nikhilsaraf Dec 8, 2017

nikhilsaraf Dec 8, 2017

nikhilsaraf Dec 8, 2017

nikhilsaraf left a comment

nikhilsaraf Dec 11, 2017

nikhilsaraf left a comment

nikhilsaraf commented Dec 12, 2017

tomerweller commented Dec 12, 2017

nullstyle Dec 13, 2017

nullstyle Dec 13, 2017

nullstyle Dec 13, 2017

nullstyle left a comment

nullstyle Dec 13, 2017

fix paging #192

fix paging #192

Conversation

tomerweller commented Dec 5, 2017

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerweller Dec 9, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilsaraf left a comment

Choose a reason for hiding this comment

nikhilsaraf commented Dec 12, 2017

tomerweller commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullstyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerweller Dec 9, 2017 •

edited

Loading