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://[email protected]/?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://[email protected]/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.

Co-authored-by: Viktor Szakats <[email protected]>

…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 <[email protected]>

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

bagder · 2024-08-13T07:36:51Z

I posted an informal poll on mastodon where right now 74% says they think we should support JSON input.
A concern I have is the JSON object: as there does not seem to be a consensus for a "URL object" in JSON, it seems reasonable to assume that other producers of JSON URLs will do it differently and therefore people will have to transpose the JSON object (using jq presumably) in order to get trurl to handle it. And if they must transpose, they could also just output a plain URL... and then, what value does JSON input add?
This PR has merge conflicts atm

SlyEcho · 2024-08-13T08:30:23Z

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);


Arguments in wrong order. From GCC 14.2.0 :

trurl.c:1744:37: error: 'calloc' sizes specified with 'sizeof' in the earlier argument and not in the later argument [-Werror=calloc-transposed-args] 1744 | char *json_string = calloc(sizeof(char), json_buf_size); | ^~~~ trurl.c:1744:37: note: earlier argument should specify number of elements, later size of each element

huh that's funny clang 15 didn't make a stink. I wonder if its a new warning in gcc 14?

either way i can fix it. good catch.

jacobmealey · 2024-08-13T15:17:14Z

Hello, sorry for letting this PR go stale. I have been thinking about this for a while. I agree with bagder and others that we don't get much from having JSON input. It just an easy feature to suggest. At first glance it makes sense that if we produce JSON we should be able to consume it, but as has been mentioned in this PR as well as the related issues, we wouldn't get much mileage out of it. The only real use case I can think of is using some external program to act as a complex filter / manipulator in between two trurl calls:

$ trurl -f some_urls.txt --json | a_very_complicated_filter.py | trurl -j - > filtered_urls.txt

but like has been said so many times this can be done by adding in a jq call to the pipeline.

Moving away from the user experience side into the technical implementation, I don't like these changes. I think the library I chose is really nice to use but it's overkill for this use case and a much simpler one should have been used. @bagder I think we should close this and open a discussion around it, i do not think this code should be merged in.

bagder · 2024-08-14T06:00:33Z

Thanks for being so open and flexible @jacobmealey, even after having spent the time doing this PR. Closing this now.

I remain unconvinced that JSON input is very useful because the reasons mentioned above.

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

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

Update .github/workflows/makefile.yml

292fd77

Co-authored-by: Viktor Szakats <[email protected]>

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 <[email protected]>

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

SlyEcho reviewed Aug 13, 2024

View reviewed changes

bagder closed this Aug 14, 2024

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

Loading

jacobmealey commented May 16, 2024 •

edited

Loading

vszakats commented May 16, 2024 •

edited

Loading

vszakats commented May 16, 2024

jacobmealey commented May 17, 2024 •

edited

Loading

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

bagder commented Aug 13, 2024

SlyEcho Aug 13, 2024

jacobmealey Aug 13, 2024

jacobmealey Aug 13, 2024

jacobmealey commented Aug 13, 2024

bagder commented Aug 14, 2024

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

Conversation

jacobmealey commented May 16, 2024 • edited Loading

jacobmealey commented May 16, 2024 • edited Loading

vszakats commented May 16, 2024 • edited Loading

vszakats commented May 16, 2024

jacobmealey commented May 17, 2024 • edited Loading

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

bagder commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobmealey commented Aug 13, 2024

bagder commented Aug 14, 2024

jacobmealey commented May 16, 2024 •

edited

Loading

jacobmealey commented May 16, 2024 •

edited

Loading

vszakats commented May 16, 2024 •

edited

Loading

jacobmealey commented May 17, 2024 •

edited

Loading