Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamically load pages based on string parameter #58

Open
frederikhors opened this issue Jul 29, 2019 · 13 comments
Open

Dynamically load pages based on string parameter #58

frederikhors opened this issue Jul 29, 2019 · 13 comments

Comments

@frederikhors
Copy link

frederikhors commented Jul 29, 2019

First of all thanks a lot for quicktemplate, I'm newbie and this is great: I'm learning a lot from your code. Thanks again!

(Maybe) a silly question: I'm trying to use authboss with quicktemplate but I don't know if I'm doing it well.

Authboss has just one interface (https://godoc.org/github.com/volatiletech/authboss/#Renderer) for its rendering system.
As stated in its README (https://github.com/volatiletech/authboss/blob/master/README.md#rendering-views):

The renderer knows how to load templates, and how to render them with some data and that's it.

...

When your app is a traditional web application and is generating it's HTML serverside using templates this becomes a small wrapper on top of your rendering setup. For example if you're using html/template then you could just use template.New() inside the Load() method and store that somewhere and call template.Execute() in the Render() method.

There is also a very basic renderer: Authboss Renderer which has some very ugly built in views and the ability to override them with your own if you don't want to integrate your own rendering system into that interface.

If you read the code for html.go you can see Load() and Render() methods.

I started copying that code and if I understand correctly:

  • the Load() method: I think I don't need it (for my basic work). Am I wrong? The original authboss one is here with some comments of mine:
func (h *HTML) Load(names ...string) error {
    if h.layout == nil {
        b, err := loadWithOverride(h.overridePath, "html-templates/layout.tpl") // I use an interface for layout page with quicktemplate
        if err != nil {
            return err
        }
        h.layout, err = template.New("").Funcs(h.funcMap).Parse(string(b)) // I don't need parsing anymore
        if err != nil {
            return errors.Wrap(err, "failed to load layout template")
        }
    }
    for _, n := range names {
        filename := fmt.Sprintf("html-templates/%s.tpl", n) // this is already an object in my code, right?
        b, err := loadWithOverride(h.overridePath, filename)
        if err != nil {
            return err
        }
        clone, err := h.layout.Clone() // this is already an object in my code, right?
        if err != nil {
            return err
        }
        _, err = clone.New("authboss").Funcs(h.funcMap).Parse(string(b)) // this is already an object in my code, right?
        if err != nil {
            return errors.Wrapf(err, "failed to load template for page %s", n)
        }
        h.templates[n] = clone // maybe something like this with functions?
    }
    return nil
}
  • for the Render() method I can use something like this:
func (h *HTML) Render(ctx context.Context, page string, data authboss.HTMLData) (output []byte, contentType string, err error) {
	// -------> Original authboss one, commented now
	// buf := &bytes.Buffer{}
	// tpl, ok := h.templates[page]
	// if !ok {
	// return nil, "", errors.Errorf("template for page %s not found", page)
	// }
	// err = tpl.Execute(buf, data)
	// if err != nil {
	// return nil, "", errors.Wrapf(err, "failed to render template for page %s", page)
	// }

	// -------> Mine
	buf := &bytes.Buffer{}
	templates.WritePage(buf, &templates.LoginPage{
		Data: data,
	})
	return buf.Bytes(), "text/html", nil
}

which has the problem I cannot dynamically load pages in templates.WritePage() method based on page parameter.

LoginPage is coming from a template like this:

{% import "github.com/volatiletech/authboss" %}

{% code
    type LoginPage struct { Data authboss.HTMLData } 
%}

{% func (p *LoginPage) Title() %}
    Login
{% endfunc %}

{% func (p *LoginPage) Body() %}
    <b>Data: {%v p.Data.Something %}</b>
    <form action="/login" method="POST">
        <input type="email">
        <input type="password">
        <button>Login</button>
        {% if p.Data["modules"] != nil %} 
            Something else with modules...
        {% endif %}
    </form>
{% endfunc %}

Maybe with reflection? Really? I read everywhere reflection is really slow and I need to use something else if possible.

I tried also with something like this:

{% import "github.com/volatiletech/authboss" %}

{% code var ALL_TEMPLATES map[string]*LoginPage %}

{% code
    func init() {
        ALL_TEMPLATES = make(map[string]*LoginPage)
        ALL_TEMPLATES["login"] = &LoginPage{}
    }
%}

{% code
    type LoginPage struct { Data authboss.HTMLData } 
%}

{% func (p *LoginPage) Title() %}
    Login
{% endfunc %}

{% func (p *LoginPage) Body() %}
    <b>Data: {%v p.Data.Something %}</b>
    <form action="/login" method="POST">
        <input type="email">
        <input type="password">
        <button>Login</button>
        {% if p.Data["modules"] != nil %} 
            Something else with modules...
        {% endif %}
    </form>
{% endfunc %}

but I think something is wrong here. I don't like ALL_TEMPLATES slice way of doing this.

What do you suggest?

I can write a Wiki (for newbies like me) in this project and in the authboss one.


I already opened an issue on authboss repo: volatiletech/authboss#239.

@valyala
Copy link
Owner

valyala commented Jul 29, 2019

You can try to put all the Write* functions from quicktemplate into a global map:

var templates = map[string]func(w io.Writer, data authboss.HTMLData) {
    "login_page": templates.WriteLoginPage,
    "main_page": templates.WriteMainPage,
}

All the quicktemplate functions must accept a single arg - data authboss.HTMLData:

{% func LoginPage(data authboss.HTMLData) %}
...
{% endfunc %}

{% func MainPage(data authboss.HTMLData) %}
...
{% endfunc %}

Then the map could be used from Render func:

var bb bytes.Buffer
f := templates[page]
f(&bb, data)
return bb.Bytes(), "text/html", nil

Probably this would work, but personally I don't like this abstraction, since it adds a level of indirection - a map of template functions - which makes code less clear. Additionally it restricts template functions to accept only a single argument - data authboss.HTMLData. Probably it would be better to don't use authboss.Renderer abstraction.

@frederikhors
Copy link
Author

frederikhors commented Jul 29, 2019

What a great honor for me to read your answer, @valyala. Thank you so much.

I thought of a similar solution (map[string]func()) in addition to the others already written before.

And I don't like it as a solution too.

Could you explain me better what you mean by:

Probably it would be better to don't use authboss.Renderer abstraction.

I think it's the only way to use authboss (with templates). Am I wrong?

@frederikhors
Copy link
Author

What do you think about the code below, @valyala?

  • Do you think it has a strong impact on memory?

  • Do you see any performance pitfall?

type AuthPage interface {
	Title() string
}

type LoginPage struct {
	Data authboss.HTMLData
}

func (lp *LoginPage) Title() string {
	return "title"
}

func InitializeLoginPage(data authboss.HTMLData) AuthPage {
	return &LoginPage{Data: data}
}

type RecoverPage struct {
	Data authboss.HTMLData
}

func (rp *RecoverPage) Title() string {
	return "title"
}

func InitializeRecoverPage(data authboss.HTMLData) AuthPage {
	return &RecoverPage{Data: data}
}

func main() {
	templates := map[string]func(authboss.HTMLData) AuthPage{
		"login":   InitializeLoginPage,
		"recover": InitializeRecoverPage,
	}
	newLoginPage := templates["login"](data)
	newRecoverPage := templates["recover"](data)
}

@valyala
Copy link
Owner

valyala commented Jul 30, 2019

Could you explain me better what you mean by:

Probably it would be better to don't use authboss.Renderer abstraction.

I think it's the only way to use authboss (with templates). Am I wrong?

I have no experience with authboss, so cannot add anything. If authboss cannot be used without authboss.Renderer abstraction, then it would be better using another package. I'd recommend starting with plain Go code without using any third-party packages. This usually ends up with clearer code, which has no superfluous abstractions and workaround hacks for external frameworks.

@frederikhors
Copy link
Author

frederikhors commented Jul 30, 2019

Ok. Thanks.

Can you tell me about the code in previous comment (#58 (comment))?

After I can close this issue.

I tried with factory pattern. What do you think?

@valyala
Copy link
Owner

valyala commented Jul 30, 2019

I tried with factory pattern. What do you think?

I think it is better to use plain old switch instead:

// WritePage write page for the given templateName and the given arg into w.
func WritePage(w io.Writer, templateName string, args interface{}) {
    switch templateName {
    case "login":
        return WriteLoginPage(w, args)
    case "recover":
        return WriteRecoverPage(w, args)
    default:
        fmt.Fprintf(w, "unknown templateName=%q", templateName)
    }
}

This switch is simpler than the map or factory method because it avoids a level of indirection or two (in case of factory method), so the code is easier to understand, update and maintain.

@frederikhors
Copy link
Author

frederikhors commented Jul 31, 2019

The problem

Yes, the problem is I need initializers because I'm using a single func:

{% interface PageImpl {
        Title()
        Body()
    }
%}

{% func Page(p PageImpl) %}
  <html>
    ...
  </html>
{% endfunc %}

and every authboss page has:

{% code
    type LoginPage struct {
        Data authboss.HTMLData
    }
%}

{% func (p *LoginPage) Title() %}
  Login
{% endfunc %}

{% func (p *LoginPage) Body() %}
  ...
{% endfunc %}

so I don't have func like WriteLoginPage() and WriteRecoverPage(): just WritePage().

What have I done

Now I'm using authboss.Renderer interface's Load() method:

type HTML struct {
  ...
  templates    map[string]func(authboss.HTMLData) templates.PageImpl
  ...
}

func InitializeLoginPageType(data authboss.HTMLData) templates.PageImpl {
	return &templates.LoginPage{Data: data}
}

func (h *HTML) Load(names ...string) error {
	for _, n := range names {
		switch n {
		case "login":
			h.templates[n] = InitializeLoginPageType
		}
	}
	return nil
}

func (h *HTML) Render(ctx context.Context, page string, data authboss.HTMLData) (output []byte, contentType string, err error) {
	buf := &bytes.Buffer{}
	tpl, ok := h.templates[page]
	if !ok {
		return nil, "", errors.Errorf("...")
	}
	templates.WritePage(buf, tpl(data))
	return buf.Bytes(), "text/html", nil
}

The million dollar question

Do you see problems with this code?

Do you see parts to improve?

@valyala
Copy link
Owner

valyala commented Aug 1, 2019

This approach looks OK from the first sight.

@frederikhors
Copy link
Author

frederikhors commented Aug 2, 2019

Your advice is invaluable, thank you very much.

I am still learning Go and every problem is an opportunity to learn, but only if I am "guided" by skilled and generous people like you.

I have created two different versions of the solution and would like to know which one is best or if there is a third one better.

  1. https://github.com/frederikhors/authbossQuicktemplate_1, factory pattern and initializators

  2. https://github.com/frederikhors/authbossQuicktemplate_2, with interface and a SetData(data authboss.HTMLData) (page PageImpl) method

I think you prefer the latter, but I don't know what to improve for performances.

  • Is it possible to do the same thing differently and less in terms of hardware resources?

  • Do you think I can improve using pointers somewhere?

@valyala
Copy link
Owner

valyala commented Aug 2, 2019

Both versions look almost identical. Take the simplest version which will be easier to understand and maintain in the future. As for the performance, both versions contain superfluous memory allocation when returning the byte slice from bytes.Buffer. The memory allocation could be avoided if Render could accept io.Writer to write template output to.

@frederikhors
Copy link
Author

The memory allocation could be avoided if Render could accept io.Writer to write template output to.

I can ask author to get rid of it.

Can I ask you what are you using for authentication when you cannot use third-party services (e.g. auth0)?

@valyala
Copy link
Owner

valyala commented Aug 5, 2019

Can I ask you what are you using for authentication when you cannot use third-party services (e.g. auth0)?

I usually abstract away the authentication into a separate package auth and then use the following code in the beginning of request handlers, which must be authenticated:

if err := auth.Authenticate(req); err != nil {
    return fmt.Errorf("authentication failure on request %s: %s", req, err)
}

@frederikhors
Copy link
Author

Ok. However, I mean the technology that you usually use as an alternative to everything that authboss makes available for free (user management with 2FA, recover, registration and so on ...).

Something like devise for Rails, I don't know if you know the library.

But I think I'm going off-topic.

Just your quick answer out of curiosity and then I close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants