is_valid_label fails to validate label type, while is_valid_label_name is calling into is_valid_label, should be the other way around

[MironAtHome]

purely coding issue.

Function is_valid_label should be validating whether label exists, inspecting label type.
In addition to label name, it should be accepting parameter graph name, as well.
Function is_valid_label_name should only check string against length and pattern, and used in is_valid_label for name part validation, prior passing request to function label_exists(label_name, graph_oid)
As a separate note, which was result of this code study, found the discrepancy between this function check approach, using macro
OidIsValid
and macro
label_id_is_valid(id) (id >= LABEL_ID_MIN && id <= LABEL_ID_MAX)
While there is a bit of difference between the two checks, in addition Oid is unsigned integer of 32 bit width, whilst label id currently defined as int32, which would make it a signed integer, which places the numeric expression for the two objects into different address space.
It would be very nice to clarify and settle on a common approach to id scheme.

[MironAtHome]

Refactored functions available in the fork, under respective branches:
branch port/win32msvc/PG14/1.5.1 is_valid_label_name
branch port/win32msvc/PG16/1.5.1 is_valid_label_name
branch port/win32msvc/PG14/1.5.1 is_valid_label
branch port/win32msvc/PG16/1.5.1 is_valid_label

[jrgemignani]

@MironAtHome The function is_valid_label is an internal function that is part of name validation. For example, it is used by is_valid_label_name. Its function is not to validate whether a label exists, rather it is to validate the proper construction of the name itself. This is why it doesn't need to know the graph name.

/*
 * Returns whether the label name is valid.
 *
 * Note: label_type parameter is not used in this implementation.
 * It should be used if validation algorithm for edge and vertex
 * differs in future.
 *
 * @param label_name name of the label
 * @param label_type label type defined in label_commands.h
 * @return int
 */
int is_valid_label(char *label_name, char label_type)

PG_FUNCTION_INFO_V1(age_is_valid_label_name);

Datum age_is_valid_label_name(PG_FUNCTION_ARGS)
{
    agtype *agt_arg = NULL;
    agtype_value *agtv_value = NULL;
    char *label_name = NULL;
    bool is_valid = false;

...
    is_valid = is_valid_label(label_name, 0);
    pfree(label_name);

    if (is_valid)
    {
        PG_RETURN_BOOL(true);
    }

    PG_RETURN_BOOL(false);
}

[jrgemignani]

@MironAtHome OidIsValid is a macro to check if an Oid is valid, and only that -

#define OidIsValid(objectId) ((bool) ((objectId) != InvalidOid))

InvalidOid is always a zero value -

#define InvalidOid ((Oid) 0)

While label_id_is_valid is validating that a label id is within a range that is within 32 bits, 16 to be precise -

#define PG_UINT16_MAX (0xFFFF)

#define LABEL_ID_MIN 1
#define LABEL_ID_MAX PG_UINT16_MAX
#define INVALID_LABEL_ID 0

#define label_id_is_valid(id) (id >= LABEL_ID_MIN && id <= LABEL_ID_MAX)

[MironAtHome]

I have 2 separate questions regarding above
#1 is_valid_label
and
#2 label_id_is_valid ( with regards to PG_UINT16_MAX )
I tried best I could to clarify both questions from the start, but it looks like things got a bit clobbered, so, let me try one more attempt. This is only to better clarify and see what your / team thinking is regarding same questions asked above.
So, let's start from question number 1 :)
Yes, there is a function is_valid_label and, just as you have pointed out, it currently does cater to label name.
However, there is one more function, and taking into account function mentioned in your response, age_is_valid_label_name, function, there are 2 functions total catering to label name validation, one internal and one exposed as part of SQL interface.
Now, if you look into file name_validation.c you will find that it currently carries 2 ( non - static ) functions:
Function number 1: is_valid_graph_name
Function number 2: is_valid_label
So, what I am trying to say is that it would be very - very nice to make functions named consistent, so that both were named according to their function, similarly, and so, after the change the two function names will be
Function number 1: is_valid_graph_name
Function number 2: is_valid_label_name
Please note, an static function with name is_valid_label currently does not exist in the unit label_commands.c
So, my suggestion is to create is_valid_label and have it cater to label validity. Whether it makes sense to make it not - static and available, as part of the C interface, it would be something to weight. On my mind it makes sense to have a function like this available throughout, with its declaration in header file.
In addition, for consistency, it would be very - very nice to add to graph_commands.c function
age_is_valid_graph_name, And if you could follow this path of thought, it would be very - very nice to add check for valid name to each function, where either external user or internal call requests some operation on graph or label name, to defend code from some request with some name, that could, potentially, cause trouble.
Please note, I am not trying to refer to something like naming convention. My hope is to instill consistency into interface.
To note, it does make sense to make functions, checking validity of name or object return boolean. Currently those return integer.
Please do accept all what I said with a grain of salt and my apologies ahead of time for redacting away in each sentence "in my opinion", else, this comment will ran twice as long :)

[MironAtHome]

Regarding question number 2 [ label_id_is_valid ( with regards to PG_UINT16_MAX ) ]
I guess, it would be very nice, if you could please help to understand, whether Age considers label id to be an Oid with range from 0 to (2^32 - 1) or something else.
It seems to me, based on your answer above "something else" is correct, but than there are two separate checks for the label id makes for a great deal of confusion.
While both checks
Check number 1: OidIsValid
Check number 2: label_id_is_valid
would render label id 0 is failing check, the two carry very different suggestion with regards to what would pass the test.
Check number 1 will pass with success from 1 to (2*32 - 1)
Check number 2 will pass from 1 to 65535
On my mind for consistency sakes it would be nice to have one check for correctness of label id.
In an ideal world what I would really like to learn from you in this question, first, before seeing any change needed or not needed, is how you see the data model, what is the true meaning of this label id,
Is it a row id of a record in edge table ( in which case it should probably be generated by a sequence, unless user wants it to be something specific, when, for example, loading from file )?
Is it a type id of a distinct edge? ( in which case this id should probably repeat in each applicable row for the id field )
I am honestly asking this question from the position of "unaware" user, who looks for guidance, in kind, before anything else.
The two different checks and the limit value set to 65535, seem to convey enough confusion, to ask for guidance first.

[jrgemignani]

@MironAtHome I can clarify more of this tomorrow, but I feel I should mention the following,...

Only functions, like below, are available to SQL or Cypher, all the others are only available for people contributing to the codebase -

PG_FUNCTION_INFO_V1(create_vlabel);

The prefix age_ is used to help eliminate naming conflicts, group our functions together, and allow them to be used in Cypher. C would have all kinds of fits if we tried to redefine an existing library function.

And, yes, it would be nice to have consistency with naming. Unfortunately, with many differently experienced cooks, come many flavors.

[jrgemignani]

@MironAtHome Also, you are welcome to submit a PR for a change to make things more consistent. So long as it builds without issues and is consistent, then it should be accepted.

[MironAtHome]

Will do.

[jrgemignani]

@MironAtHome Given the size of our team, those who actively contribute to the project, the size of the codebase, it can be difficult to enforce naming conventions and where functions are located. And, of course, get anything done at all :) This is especially true when it comes to developer experience. Meaning, knowing where something 1) should be, 2) if it exists elsewhere, or 3) the proper way to build it in PG and AGE. So, I welcome people who want to contribute and bring our codebase more into line when they find these issues.

If you feel a change would be useful, please provide a PR for the master branch, for review, and we can move on from there.

edit: and even experienced devs make mistakes.

[jrgemignani]

Changed to enhancement as this isn't really a bug.

[MironAtHome]

Thanks!