[#465] Replace cJSON dependancy with the native in-tree json API

[MohanadKh03]

Replaced cJSON completely with the native json parser.
Tested manually using the following commands from the docs user guide section 6.4 JSON Output Format

pgagroal-cli conf ls --format json

pgagroal-cli conf set max_connections 1000 --format json

pgagroal-cli status --format json

pgagroal-cli ping --format json

Though I noticed that when using text format it gave a status of 1 and exits for some reason ?
After debugging I think the problem is that some exit status is not what it should be I think ? I am kind of stuck in it cuz now it's only working with json formats not text somehow

[jesperpedersen]

Start by putting the definitions into MANAGEMENT_ARGUMENT_XYZ defines.

Also, look at the return values from the functions so f.ex. .._json_create should result into a goto error: if it fails.

But, you are close on this !

[MohanadKh03]

Ok so appearantly the issue was the way printing using text was coupled with cJSON which would have wasted a lot of time and effort to debug on it so instead I used the native API's way --> pgagroal_json_print_and_free_json_object and renamed it cuz it will have both json and text and possibly any other format in the future ?

As for the definitions , not sure if I got this right but I tried moving the functions to the management.h file though they are static so do you mean to remove them being static or move some other functions ?

Added another commit , I know I should squash but for the sake of readability since this has a lot of changes I let it like this till all is good then ill squash
WDYT ?

Update:
CI failing because of missing memory leaks management . fixed that but it still is failing not sure why ? even though it's working when using make locally using gcc though the CI uses clang i think

[jesperpedersen]

See CI - always init to NULL then assign after

[jesperpedersen]

@MohanadKh03 See #466 which is next, and how it is done in pgmoneta.

You can just follow cli.c -> management.c -> main.c -> ?

[jesperpedersen]

@MohanadKh03 You can do both at the same time if that is easier... and you want to

[MohanadKh03]

It's all good I will fix the memory leaks issue and continue with this one before getting to the 466
@jesperpedersen Quick question though why did the leak appear only on the linux build ? Also there seems to be a lot of stuff that are not freed at the end of lots of functions even before this PR's changes so does this have to do with sth else not memory leaks ?
CI says AddressSanitizer: 6 byte(s) leaked in 1 allocation(s). which is understandable there is indeed sth that is not freed up but this has been around for a while so why did it say there were 6 bytes leaked now ?

if (pgagroal_executable_version_string(&version_buf, version))
  {
     goto error;
  }

This is the end of the function which has been like this before my changes so im kinda confused why did it detect a leak only now not before ?

free(buffer);
return json;
error:
free(buffer);
return NULL;

[jesperpedersen]

Maybe it depends on the path of the code in question, and likely the Mac builds are using much older software.

I use clang 18.x w/ Debug to catch most of them - valgrind is too slow

[jesperpedersen]

... and GitHub have seemed to update their platform yesterday so more reports are coming in now - I'm guessing the ubuntu-latest image is different now

[fluca1978]

I think the work is very good, but the main point I don't like at all is the printing out the whole json stuff in a text tree when the output is specified as text and the verbosity flag is turned on. While text output is not supposed to be stable, do we need to append such a bulk of information?

Second, I'm not sure the idea naming "possibly handling other formats in the future" functions without the json suffix really works: as far as I understand we will use json as the backbone format from now on, so make it clear a function works on json even when is dealing with another format.

[fluca1978]

I'm not sure this function should not have json in its name. While I agree that we could have more formats in the future, the function will always print from a json object and free a json object. The printing could be in YAML, as an example, but we start and free always a json struct, right?

[MohanadKh03]

Yeah I noticed json was the main format in everything even when printing out text it was basically extracted from json. Makes sense

[fluca1978]

This is much too information according to me.
If verbose is specified, the previous version of the application was printing the result and the exit status, e.g.:

% pgagroal-cli conf get max_connections --verbose
max_connections = 10
pgagroal-cli: Success (0)

Now it is printing out the whole json, that is somehow confusing and difficult to parse:

% pgagroal-cli conf get max_connections --verbose
application: 
  major: 2
  minor: 0
  name: pgagroal
  patch: 0
  version: 2.0.0
command: 
  error: 0
  exit-status: 0
  name: conf get
  output: 
    key: max_connections
    value: 10
  status: OK
pgagroal-cli: Success (0)

[jesperpedersen]

I think it about getting the conf get | set ported to pgmoneta, and then we can work from there

@Jubilee101 Cc:

[jesperpedersen]

pgmoneta/pgmoneta#401

[MohanadKh03]

True I actually thought that by verbose it meant literally verbose to print out everything out but now I get it
The second commit is actually more of a trial-and-error tbh but ill rewrite the logic of text format again starting the first commit as a fresh one now that I understand most of the logic
Though in status details for example isnt it normal to have all the details so the whole object should be printed out right ?

[jesperpedersen]

@MohanadKh03 Maybe we should start with pgmoneta's 401 to figure out the best way to extract a single value, or set it...

Because the changes will likely be part of pgagroal's 466 anyway

[fluca1978]

True I actually thought that by verbose it meant literally verbose to print out everything out but now I get it The second commit is actually more of a trial-and-error tbh but ill rewrite the logic of text format again starting the first commit as a fresh one now that I understand most of the logic Though in status details for example isnt it normal to have all the details so the whole object should be printed out right ?

Well, details is a very rich command, and it is text printed in a kind of tabular data. We could simplify the output by simply printing out the json object, but I would not change the current text format too much in order to not "scary" users that could have been scripting on the text output.

That's also why --verbose should not be the implementation of a dump command, according to me.

[jesperpedersen]

@fluca1978 Take a look at the output from pgmoneta - it is A LOT easier to maintain. And, this is a 2.0 release, so I/O and management will change... maybe people may just use tools like jq

[MohanadKh03]

@jesperpedersen After reading pgmoneta's way imo it's much easier to read and maintain tbh compared to the current pgagroal's way

So do you suggest holding this PR off until 401 in pgmoneta is done and then we can standardize the IO,Management in both pgmoneta and pgagroal ?

[jesperpedersen]

@MohanadKh03 Yes, the management part should be unified across pgagroal, pgmoneta and pgexporter.

@decarv is taking care of the I/O part