Add --json-file/-j option to read from file w/ json contents #311

jacobmealey · 2024-05-16T13:19:07Z

This adds --json-file as an option to read from a file that contains a json representation of the urls. It works very similarly to --url-file. The --json-file input structure is designed to match roughly what the output of --json produces. the simplest json object possible is:

[{ "parts": { "host": "example.com" }}]

You can pass an arbitrary number of these url JSON objects and perform operations on them like you would with any other url in trurl.

Something that I did that may be up for debate is how the query is handled. I chose to make the query only be able to be set by an array of "keys" and "values" (the "params" section of --json") and it will throw a warning if the "query" component of "parts" is set.

A few examples:

$ cat testfile/test004.txt
[
  {
    "parts": {
      "scheme": "ftp",
      "user": "scream",
      "host": "url.com",
      "path": "/",
      "query": "ignoredquery",
      "fragment": "a-fragment"
    },
    "params": [
      {
        "key": "query",
        "value": "pair"
      },
      {
        "key": "singlequery",
      }
    ]
  },
  {
    "parts": {
      "scheme": "http",
      "host": "example.org",
      "path": "/"
    }
  }
]
$ trurl -j testfiles/test0004.txt --set "fragment=a-new-fragment" 
trurl note: ignoring 'query', provide a separate 'params' array.
ftp://scream@url.com/?query=pair&singlequery#a-new-fragment
http://example.org/#a-new-fragment
$ trurl -j testfiles/test0004.txt --append "path=file"
trurl note: ignoring 'query', provide a separate 'params' array.
ftp://scream@url.com/file?query=pair&singlequery#a-fragment
http://example.org/file

Currently the feature is disabled by default, do turn it on you need to add TRURL_JSON_IN to your environment to enable it.

fixes: #291

brew install...

not leaking

cleanup action cleanup trurl.1 cleanup trurl.1

jacobmealey · 2024-05-16T13:40:59Z

okay I need to figure out whats going on with the windows builds, the cygwin was was working on my local branch until very recently, might be something i changed on the way we are doing the file i/o. I will take a closer look this evening.

Edit: Okay, i got the cygwin build working. I just need to add libjson-c to the curl-for-win build. @vszakats would this be done by manually compiling and linking it in here: https://github.com/curl/curl-for-win/blob/main/trurl.sh ?

vszakats · 2024-05-16T21:14:35Z

It's a new dependency, kind of a big deal to add support for one in general. One approach is to make it a build option, disabled by default, so existing trurl builds (including curl-for-win) continue to work without modification. Another option is to delete/disable the curl-for-win CI job till I (or someone else) get around adding support for it. A 3rd one is to add an option to disable this feature, and I update curl-for-win to set it.

vszakats · 2024-05-16T21:29:28Z

...that said, have you thought of using a JSON parser that's a self-contained C89 source (with a fitting license)? It's still a dependency, but perhaps easier to integrate and carry.

jacobmealey · 2024-05-17T12:22:53Z

have you thought of using a JSON parser that's a self-contained C89 source

I was looking a lot at https://github.com/DaveGamble/cJSON, but I didn't like how it looks and json-c has some nice features. Both this and json-c are in vcpkg, freebsd repos, and most linux distro repositories so i was hoping it wouldn't be too much work either way for maintainers.

If folks want a smaller ANSI c json library instead of the one I chose, I think that's a fair request and I can start working this again.

for now, I will put it behind a compile time flag as you suggested

Edit:
There are a ton of other libraries listed on json.org that could be used too.

.github/workflows/makefile.yml

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

.github/workflows/makefile.yml

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

.github/workflows/makefile.yml

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

…rl-json-in

bagder

We made the -f function possible to work as a filter, as in it reads URLs and outputs the results in the mean time. This JSON reader reads the entire file before it acts on it, which makes it not work as a filter and if you want to work an a huge number of JSON URLs this will allocate a lot of memory...

trurl.c

jacobmealey · 2024-05-24T16:42:08Z

@bagder, i've made a few big modifications. Mainly I made it so it reads one json object at a time, so it now only allocates as much memory as required to parse the largest single json object, instead of attempting to read until the of the file. I also made it so it reads in a fixed buffer on the stack, and then allocates more data on the heap if need be as you suggested.

Makefile

.github/workflows/makefile.yml

Makefile

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

Makefile

sevan · 2024-06-11T22:06:04Z

Makefile

@@ -33,6 +33,14 @@ CFLAGS += -Wconversion -Wmissing-prototypes -Wwrite-strings -Wsign-compare -Wno-
 ifndef NDEBUG
 CFLAGS += -g
 endif
+ifdef TRURL_JSON_IN
+CFLAGS += -DTRURL_JSON_IN -Wno-gnu


I couldn't find any info on -Wno-gnu is it necessary? because it breaks the build on older GCCs.

hm thats not good. It was required by json-c , I think some of there macros are what required this. I'm wondering if we should jump out of json-c, and use one that is purely c89 (and small enough we can just include the source in the repo or something like @vszakats has been saying)

I think a lot of the scaffolding can stay in place for parsing single objects / streaming stuff. it would really just be the guts of from_json() that would need to be swapped out.

What OS / compiler have you been using?

sevan · 2024-06-11T22:11:31Z

With libjson-c installed (I tested against v0.17), I was able to build your branch using make TRURL_JSON_IN=1 JSON_C_PREFIX=/bla

Worth setting JSON_C_PREFIX ?= /usr/local in Makefile so it only requires make TRURL_JSON_IN=1? (PREFIX also defaults to /usr/local)

bagder · 2024-06-12T07:47:28Z

trurl.c

+    *usedarg = gap;
+    o->json_in = true;
+#else
+    trurl_warnf(o, "not built with support for JSON input.");


I propose using errorf() here instead.

bagder · 2024-06-12T07:50:05Z

trurl.c

+  if(json_object_object_get_ex(wholeurl, "params", &params)) {
+    size_t params_length = json_object_array_length(params);
+    for(size_t j = 0; j < params_length; j++) {
+      json_object *param = json_object_array_get_idx(params, (int)j);


Can't this function call fail and return NULL? Shouldn't this exit if so?

bagder · 2024-06-12T07:50:34Z

trurl.c

+        qpair[key_length] = '=';
+        memcpy(qpair + key_length + 1, param_v, value_length);
+      }
+      this_query = realloc(this_query, this_q_size + qpair_len);


No check if this returns NULL.

bagder · 2024-06-12T07:50:43Z

trurl.c

+  if(this_q_size) {
+    this_query[this_q_size - 1] = '\0';
+    const char *qss = "query:="; /* do not encode the url */
+    char *query_set_str = malloc(sizeof(char) * (this_q_size + strlen(qss)));


bagder · 2024-06-12T07:50:59Z

trurl.c

+    size_t key_len = strlen(key);
+    size_t val_len = strlen(val_str);
+    /* +2, one char for '=' and one for null terminator. */
+    char *set_str = malloc(val_len + key_len + 2);


bagder · 2024-06-12T07:52:35Z

trurl.c

+    size_t val_len = strlen(val_str);
+    /* +2, one char for '=' and one for null terminator. */
+    char *set_str = malloc(val_len + key_len + 2);
+    memset(set_str, 0, val_len + key_len + 2);


Why clear it first if you copy over the contents immediately below?

bagder · 2024-06-12T07:54:07Z

trurl.c

+    free(set_str);
+  }
+  if(!scheme_set) {
+    setone(uh, "scheme=http", o);


This makes it differ from how it acts on setting a plain URL. Then we let libcurl "guess" the scheme, which may not always be HTTP...

okay, I thought we just defaulted to HTTP. The way this is working is its constructing a CURLU with curl_url_set() via setone. From this we are setting setting every component manually and if we don't set a scheme libcurl throws an error not enough input for a url.

Maybe the approach i took was a bit too low level. If instead we just concatenate all the components together in parts and pass that string to single url then it would just do all the scheme guessing.

bagder · 2024-06-12T07:55:12Z

trurl.c

+  if(!scheme_set) {
+    setone(uh, "scheme=http", o);
+  }
+  struct iterinfo iinfo;


Declaring a variable in the middle of a block is not C89 compliant. I believe we still try to keep it C89...?

From past experience with GCC 4.0.1, no, the way loops are done in the code base assume C99, which is why I added -std=gnu99 to CFLAGS in the past (now gone, not a problem). If things are meant to build with a C89 compiler, I can raise issues for that separately as I have ancient GCC at hand to attempt things.

bagder · 2024-06-12T07:55:27Z

trurl.c

+  char reading_buff[JSON_INIT_SIZE];
+  size_t last_write = 0;
+  size_t json_buf_size = JSON_INIT_SIZE;
+  char *json_string = calloc(sizeof(char), json_buf_size);


no error check

bagder · 2024-06-12T07:56:42Z

trurl.c

+  size_t last_write = 0;
+  size_t json_buf_size = JSON_INIT_SIZE;
+  char *json_string = calloc(sizeof(char), json_buf_size);
+  memset(reading_buff, 0, sizeof(char) * JSON_INIT_SIZE);


Why clear this? It seems unnecessary.

bagder · 2024-06-12T12:12:17Z

tests.json

+      "input": {
+          "arguments": [
+              "-j",
+              "testfiles/test0001.txt"


testfiles/test0001.txt is not in this PR

jacobmealey added 25 commits May 16, 2024 08:09

wip: simple json handling - needs big clean up

0fbb22d

clean up source code

9e2f4a4

added more tests, need to add special handling for queries

0bd91b6

finalized testing and found a bug where querys all stack together

0e22840

query strings generating. need to fix for iteration

e01fd50

cleaning up for pr

3d55d30

test w/ actions

dcea792

Added sudo back

41bb29e

put json objects

18ecad7

remove putting whole object?

29de2be

removed whole line

a1d0ed8

fixed memory issues

ba76262

remove sanitizer

5618105

spelling

1d9d70d

attempting to make everything play nice

1685194

fixed typo

0785c94

brew install...

different packages and paths

5264619

cleaned up tests

20c3b8c

removed mac os specifics from makefile, clean style

73b272a

remove params putting

95fa596

not leaking

26e4859

not leaking

free this_query

bb1a0e3

clean up for pr

373bec0

cleanup action cleanup trurl.1 cleanup trurl.1

cleanup file handling

1851fee

get other actions working -- need to get curl for win working

cc74cdf

undo file cleanup... doesn't work on windows

b44e296

vszakats reviewed May 17, 2024

View reviewed changes

.github/workflows/makefile.yml Outdated Show resolved Hide resolved

white space Update .github/workflows/makefile.yml

12e6725

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

vszakats reviewed May 17, 2024

View reviewed changes

.github/workflows/makefile.yml Outdated Show resolved Hide resolved

Update homebrew includes .github/workflows/makefile.yml

6eb908e

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

vszakats reviewed May 17, 2024

View reviewed changes

.github/workflows/makefile.yml Outdated Show resolved Hide resolved

jacobmealey and others added 3 commits May 17, 2024 11:36

Update .github/workflows/makefile.yml

292fd77

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

make --json-file act like other optional builds

a8dad0a

Merge branch 'trurl-json-in' of github.com:jacobmealey/urler into tru…

ad93491

…rl-json-in

bagder reviewed May 18, 2024

View reviewed changes

trurl.c Outdated Show resolved Hide resolved

trurl.c Outdated Show resolved Hide resolved

Make it act as a filter

aa39f61

jacobmealey added 3 commits May 24, 2024 14:02

added error check for parts key

e5e2429

style

ede3153

update oom messaging

aa7d58b

vszakats reviewed May 24, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

vszakats reviewed May 24, 2024

View reviewed changes

.github/workflows/makefile.yml Outdated Show resolved Hide resolved

cleanup makefile

d2b3fca

vszakats reviewed May 24, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jacobmealey and others added 2 commits May 24, 2024 16:12

Update Makefile

2f81538

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

flip around the lines

3975591

sevan reviewed Jun 10, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jacobmealey added 4 commits June 11, 2024 07:52

makefile JSON_C_PREFIX, update readme

1c22c47

cleanup readme

e6f135e

trying setting json_c_path in GITHUB_ENV

a947832

prefix not path

7502449

jacobmealey requested a review from sevan June 11, 2024 12:25

sevan reviewed Jun 11, 2024

View reviewed changes

bagder reviewed Jun 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --json-file/-j option to read from file w/ json contents #311

Add --json-file/-j option to read from file w/ json contents #311

jacobmealey commented May 16, 2024 •

edited

jacobmealey commented May 16, 2024 •

edited

vszakats commented May 16, 2024 •

edited

vszakats commented May 16, 2024

jacobmealey commented May 17, 2024 •

edited

bagder left a comment

jacobmealey commented May 24, 2024

sevan Jun 11, 2024

jacobmealey Jun 12, 2024

sevan Jun 12, 2024

sevan commented Jun 11, 2024

bagder Jun 12, 2024

bagder Jun 12, 2024

bagder Jun 12, 2024

bagder Jun 12, 2024

bagder Jun 12, 2024

bagder Jun 12, 2024

bagder Jun 12, 2024

jacobmealey Jun 12, 2024

bagder Jun 12, 2024

sevan Jun 12, 2024

bagder Jun 12, 2024

bagder Jun 12, 2024

bagder Jun 12, 2024

Add --json-file/-j option to read from file w/ json contents #311

Are you sure you want to change the base?

Add --json-file/-j option to read from file w/ json contents #311

Conversation

jacobmealey commented May 16, 2024 • edited

jacobmealey commented May 16, 2024 • edited

vszakats commented May 16, 2024 • edited

vszakats commented May 16, 2024

jacobmealey commented May 17, 2024 • edited

bagder left a comment

Choose a reason for hiding this comment

jacobmealey commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sevan commented Jun 11, 2024

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

Choose a reason for hiding this comment

jacobmealey commented May 16, 2024 •

edited

jacobmealey commented May 16, 2024 •

edited

vszakats commented May 16, 2024 •

edited

jacobmealey commented May 17, 2024 •

edited