Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: olegrok@tarantool.org, tsafin@tarantool.org
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] exports: allow to use json tools via FFI
Date: Fri, 21 Aug 2020 23:56:58 +0200	[thread overview]
Message-ID: <8b318f99-f2f3-9671-96e6-0119781c39d1@tarantool.org> (raw)
In-Reply-To: <20200821162607.68179-1-olegrok@tarantool.org>

Hi! Thanks for the patch!

On 21.08.2020 18:26, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> This patch exports some json functions to be used via FFI. We solve
> following problem: currently we don't have any tools to inspect
> jsonpaths. E.g. user wants to ban or restrict an access to some
> tuple fields, it could be some system fields. Tarantool doesn't
> have hidden fields and to solve such problem we should fairly
> parse input jsonpaths. Before this patch user should write its own
> or some external tools. This patch allows to use functions from
> built-in json-lexer directly from Tarantool via FFI.

I understand the problem - you don't want to implement your own tool,
but you must realize, that exporting some internal structs and functions
via FFI is not safe. We won't care about "backward compatibility" of
internal functions. And JSON code is quite mutable, it may change in
future, if we will rework multikeys, for example. And that will make
your code unusable on newer versions. Even probably inside one version,
because we won't bump version number just for the sake of internal code
change.

Even worse is that you took not only general stuff like json_lexer_next_token,
but also json_path_multikey_offset - this is heavily internal, why do you
need it?

I can't upvote this patch, sorry. I propose you to implement a JSON path
parser in Lua, or copy-paste the C code from json.h/c. and create your
own C module. The whole json.c is 666 lines, including the multikey
support and json trees, which occupy most of the space and you don't need
them. The parser is trivial, one day work to implement it from the scratch
with the tests. Please, don't use the internal structs and functions.

You can't just export everything you need if by luck it happened we have it
somewhere internally implemented in C. It is not safe.

The only thing you need to implement the same JSON path parser is utf8 module,
and you have it.

  parent reply	other threads:[~2020-08-21 21:57 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-21 16:26 olegrok
2020-08-21 16:50 ` Timur Safin
2020-08-21 21:56 ` Vladislav Shpilevoy [this message]
2020-08-24  7:12   ` Oleg Babin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8b318f99-f2f3-9671-96e6-0119781c39d1@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] exports: allow to use json tools via FFI' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox