Summary

Remove gg_oauth_parameter_t

Review Request #516 — Created Feb. 15, 2021 and discarded Feb. 25, 2021, 12:12 p.m.

Information

Owner

qarkai

Repository

pidgin/pidgin

Branch

default

Bugs

Depends On

Reviewers

Groups

pidgin

People

Description

Use variadic function gg_oauth_generate_request instead.

Testing Done

Compile.

Issues

Description	From	Last Updated
Since gg_oauth_parameter_set is only called in a block with gg_oauth_parameter_join right after it, I feel like a variadic gg_oath_parameter_generate(gboolean header, …	QuLogic	Feb. 18, 2021, 10:15 a.m.
Mark with G_GNUC_NULL_TERMINATED	QuLogic	Feb. 19, 2021, 10:40 p.m.
And then add NULL at the end.	QuLogic	Feb. 19, 2021, 10:40 p.m.
Ditto.	QuLogic	Feb. 19, 2021, 10:40 p.m.
A comment about how this works, ie returning a comma separated header value if header is true, or a url-encoded …	grim	Feb. 21, 2021, 11:08 a.m.
curious why g_string_append_printf wasn't used?	grim	Feb. 21, 2021, 11:08 a.m.
This might read a bit better and lead to less errors later as right now there's 3 checks for header …	grim	Feb. 21, 2021, 11:08 a.m.
We can but I don't see how g_string_truncate() can lead to memory errors.	qarkai	Feb. 21, 2021, 8:20 a.m.
I just realized this is to handel the trailing , or & from line 83. This can lead to memory …	grim	Feb. 21, 2021, 11:09 a.m.
I was thinking of something more like this for this function... static char * gg_oauth_generate_request(gboolean header, ...) { GString *res …	grim	Feb. 25, 2021, 11:17 a.m.

Change Summary:

Remove oauth-parameter.

Description:

~		Replace gg_oauth_parameter with GList of PurpleKeyValuePair.
	~	Replace gg_oauth_parameter with GList of PurpleKeyValuePair.
	+	Remove oauth-parameter.

Commit:

-	4c5b90a43ac2
+	c43682760465

Diff:

Revision 2 (+69 -217)

Show changes

	libpurple/protocols/gg/meson.build
	libpurple/protocols/gg/oauth/oauth.c
	libpurple/protocols/gg/oauth/oauth-parameter.h
	libpurple/protocols/gg/oauth/oauth-parameter.c

libpurple/protocols/gg/oauth/oauth.c (Diff revision 2)

Show all issues

Since gg_oauth_parameter_set is only called in a block with gg_oauth_parameter_join right after it, I feel like a variadic gg_oath_parameter_generate(gboolean header, ...) might be simpler.

Summary:

-	Replace gg_oauth_parameter with GList of PurpleKeyValuePair
+	Remove gg_oauth_parameter_t

Description:

~		Replace gg_oauth_parameter with GList of PurpleKeyValuePair.
	~	Use variadic function `gg_oauth_generate_request` instead.
-		Remove oauth-parameter.

Testing Done:

Compile.

Commit:

-	c43682760465
+	2bd331397a0e

Diff:

Revision 3 (+54 -224)

Show changes

	libpurple/protocols/gg/meson.build
	libpurple/protocols/gg/oauth/oauth.c
	libpurple/protocols/gg/oauth/oauth-parameter.h
	libpurple/protocols/gg/oauth/oauth-parameter.c

libpurple/protocols/gg/oauth/oauth.c (Diff revision 3)
Show all issues
```
Mark with G_GNUC_NULL_TERMINATED
```
libpurple/protocols/gg/oauth/oauth.c (Diff revision 3)
Show all issues
```
And then add NULL at the end.
```
libpurple/protocols/gg/oauth/oauth.c (Diff revision 3)
Show all issues
```
Ditto.
```

Commit:

-	2bd331397a0e
+	ce354e51542e

Diff:

Revision 4 (+58 -224)

Show changes

	libpurple/protocols/gg/meson.build
	libpurple/protocols/gg/oauth/oauth.c
	libpurple/protocols/gg/oauth/oauth-parameter.h
	libpurple/protocols/gg/oauth/oauth-parameter.c

Ship it!

```
Ship It!
```

libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)

Show all issues

A comment about how this works, ie returning a comma separated header value if header is true, or a url-encoded value would be nice.

libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)
Show all issues
```
curious why g_string_append_printf wasn't used?
```

libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)

Show all issues

This might read a bit better and lead to less errors later as right now there's 3 checks for header to be true.

escaped = g_uri_escape_string(value, NULL, FALSE);
if(header) {
    g_string_append_printf(res, "\"%s\",", escaped);
} else {
    g_string_append_printf(res, "%s&", escaped);
}
g_free(escaped);

libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)

Show all issues

I just realized this is to handel the trailing , or & from line 83. This can lead to memory errors. Can we instead have a boolean to say whether or not we've processed the first item, and if so, then prefix it.

qarkai Feb. 21, 2021, 8:20 a.m.

We can but I don't see how g_string_truncate() can lead to memory errors.

grim Feb. 21, 2021, 8:38 a.m.

While this shouldn't happen, it is currently 100% possible to call

gg_oauth_generate_request(TRUE, NULL);


After looking at the implementation of g_string_truncate this won't cause a memory error, but consumers of our own internal api's shouldn't need to know the implemention of another library's api.

GString *
g_string_truncate (GString *string,
                   gsize    len)
{
  g_return_val_if_fail (string != NULL, NULL);

  string->len = MIN (len, string->len);
  string->str[string->len] = 0;

  return string;
}

qarkai Feb. 21, 2021, 11:09 a.m.
```
Added checks around g_string_truncate.
```

libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)

Show all issues

We can but I don't see how g_string_truncate() can lead to memory errors.

Change Summary:

Reduce header checks.

Commit:

-	ce354e51542e
+	094418b63495

Diff:

Revision 5 (+59 -224)

Show changes

	libpurple/protocols/gg/meson.build
	libpurple/protocols/gg/oauth/oauth.c
	libpurple/protocols/gg/oauth/oauth-parameter.h
	libpurple/protocols/gg/oauth/oauth-parameter.c

Fix it!

libpurple/protocols/gg/oauth/oauth.c (Diff revision 5)

Show all issues

I was thinking of something more like this for this function...

static char *
gg_oauth_generate_request(gboolean header, ...) {
    GString *res = g_string_new(NULL);
    va_list params;

    if(header) {
        res = g_string_append(res, "OAuth ");
    }

    va_start(params, header);
    while((key = va_arg(params, const gchar *))) {
        const gchar *value = va_arg(params, const gchar *);
        gchar *escaped = g_uri_escape_string(value, NULL, FALSE);

        if(header) {
            g_string_append_printf(res, "\"%s\",", escaped);
        } else {
            g_string_append_printf(res, "%s&", escaped);
        }

        g_free(escaped);
    }
    va_end(params);

    if(res->len > 0 && res->str[res->len - 1] == separator) {
        /* remove trailing separator */
        res = g_string_truncate(res, res->len - 1);
    }

    return g_string_free(res, FALSE);
}

grim Feb. 25, 2021, 11:26 a.m.

I forgot to drop the separator variable in the cleanup part which is still a problem if it's called with (FALSE, NULL) as well as (TRUE, NULL) where you'll end up with OAuth without a trailing space, but that's probably fine.

qarkai Feb. 25, 2021, 11:27 a.m.

This has redundant header comparison in each loop iteration. Also for me "\"%s\"," looks unclear and ugly.

qarkai Feb. 25, 2021, 11:33 a.m.

which is still a problem

It's not a problem if separator is initialized. If it's called as (header, NULL) then res's last char is space which is not equal to separator being , or & so nothing truncated.

grim Feb. 25, 2021, 11:34 a.m.

Well, before there were 3 redundant header checks in the loop and that was my initial issue. Regardless the compiler will most likely optimize them out, but my concern is about the readability of the code.

Adding additional variables isn't a good fix either as it's more stuff that someone has to remember while reading the code.

As for the escaped quotes, that's just a fact of life in a format string. Making it a variable adds additional dependent format variables for which over complicates the entire thing. It's more error prone and then you have to understand what %s=%s%s%s%c means which is even more intimidating.

Also I just noticed I missed the key part. My fault.

grim Feb. 25, 2021, 11:35 a.m.

Look here's the thing... I'm pushing to make this code base more readable. Additional variables and format strings like %s=%s%s%s%c is not what I consider more readable. I can either accept this as is and then change it myself, or we can change it here.

qarkai Feb. 25, 2021, 11:52 a.m.

Agree to disagree about readability. Feel free to change but I won't fix it, sorry.

grim Feb. 25, 2021, 11:58 a.m.

I fear this may have been taken personal, so to be clear, this is nothing personal, it is 100% about simplifying code. You're free to your opinion, but ultimately the decision comes to me. I'm sorry you feel so strongly about this, but I'm not going to change my position here and I hope this doesn't stop you from contributing in the future.

qarkai Feb. 25, 2021, 12:05 p.m.

I totally understand and it's ok to disagree. After all you are project's maintainer and last decision is yours.

Status: Discarded

Change Summary:

superceded by https://reviews.imfreedom.org/r/525/