Fixes #30: support object field #47

dcrissman · 2015-01-27T20:22:21Z

No description provided.

…t fields and use Paths over fieldName fix crud errors so that they actually are sent to the client

coveralls · 2015-01-27T20:29:31Z

Coverage increased (+0.63%) to 49.4% when pulling 334cde5 on dcrissman:issue30-support-object-field into f67a2c7 on lightblue-platform:master.

dcrissman · 2015-01-27T20:34:52Z

Ready for review

dcrissman · 2015-01-27T21:03:08Z

@bserdar - dcrissman, re: LdapFieldNameTranslator: the second method should also be updated to return a Path
@bserdar - dcrissman, you have two methods, one of them gets a path, return a string. The other should get a string, return a path.

@bserdar - dcrissman, re: doc error handling: constructing a new Doc won't work. The point of associating errors with docs was that there should be some basic identifying information about the doc with errors, so the client would know which doc has those errors. Sending an empty doc with an error array is pointless. Either don't return doc errors and fail the call, or put some identifying information to the document wiht error

dcrissman · 2015-01-28T14:06:08Z

@bserdar I changed LdapFieldNameTranslator#translateAttributeName to now return a path.

Regarding your second point about the doc error, at the point in the Find CRUD operation, there is no document to send back as we failed to translate the ldap results into a document. I would think some kind of error message would be better than nothing. Just to see what the output would be, I threw a fake RuntimeException in the code to generate the below error message. The msg would at least give a clue what happened. My other thought would be to just grab the dn off the ldap result record and add that to the returned new errored DocCtx, but I don't particularly like that as it is the ResultTranslator's job to know how to translate results, not the LdapCRUDController's. And what if part of the problem was translating the dn?

{"errors":[{"objectType":"error","context":"find(person:1.0.0)","errorCode":"RuntimeException","msg":"fake exception"}]}

coveralls · 2015-01-28T14:25:34Z

Coverage increased (+0.63%) to 49.4% when pulling 459ff20 on dcrissman:issue30-support-object-field into f67a2c7 on lightblue-platform:master.

bserdar · 2015-01-28T14:45:44Z

I think the better way to deal with this is to add translation exceptions
to context's error list (not dataError). You wouldn't return any results.

On Wed, Jan 28, 2015 at 7:06 AM, Dennis Crissman [email protected]
wrote:

@bserdar https://github.com/bserdar I changed
LdapFieldNameTranslator#translateAttributeName to now return a path.

Regarding your second point about the doc error, at the point in the Find
CRUD operation, there is no document to send back as we failed to translate
the ldap results into a document. I would think some kind of error message
would be better than nothing. Just to see what the output would be, I threw
a fake RuntimeException in the code to generate the below error message.
The msg would at least give a clue what happened. My other thought would be
to just grab the dn off the ldap result record and add that to the returned
new errored DocCtx, but I don't particularly like that as it is the
ResultTranslator's job to know how to translate results, not the
LdapCRUDController's. And what if part of the problem was translating the
dn?

{"errors":[{"objectType":"error","context":"find(person:1.0.0)","errorCode":"RuntimeException","msg":"fake exception"}]}

—
Reply to this email directly or view it on GitHub
#47 (comment)
.

bserdar · 2015-01-28T18:07:10Z

EntryBuilder:

You have to rethink the method signatures. If you pass a Field subclass, you don't need to pass Path, field.getFullPath is always path. Passing both gives the impression, and the potential for those two to be different.
Throw an error in translateObjectArray, define a const for UnsupportedFeature:objectArray, and give the field name as error msg
If you don't have this: use Error.push at the begining of translation, and pop when done

LdapCRUDController:

#131: throw error
#236: instead of doc errors, put errors into ctx.errors
#383: use existing path modification apis:

node.mutableCopy().setLast(createArrayFieldName...).immutableCopy()

TranslatorFromJson:

USe Error.push/pop at the entry method
#141: throw Error

TrivialTranslator:
#34: path.getLast() or path.toString()?

LdapMetadata:

Why path.getLast at Minor enhancements #54? Why not path.toString()?

coveralls · 2015-01-28T21:24:08Z

Coverage increased (+1.36%) to 50.13% when pulling 4097c7e on dcrissman:issue30-support-object-field into f67a2c7 on lightblue-platform:master.

dcrissman · 2015-01-28T21:36:10Z

@bserdar I believe I have address all your concerns.

Regarding TrivialTranslator and LdapMetadata, path.getLast() is needed when handling fields within an object. TrivialTranslator blindly grabs last, because by definition if it is being used than no aliases were created.

bserdar · 2015-01-29T05:36:21Z

The pattern to use Error.push/pop is:

Error.push(stuff);
try {
do stuff
} finally {
Error.pop();
}

In EntryBuilder.build, do this:

Error.push("EntryBuilder.build")
Error.push(dn);
try {
...
} finally {
Error.pop();
Error.pop();
}

Throwing errors:

EntryBuilder.82

Use the naming conventions we estanbisled for this. Look at, for instance, MetadataConstants.java. Your error should mean something to the caller. Let the caller translate the error code to a meaningful message.

For instance: LDAPCRUDController#131: I believe there is already an error code constant for missing required field, either in CrudConstants or MetadataConstants

beforeCreateNewSchema:

you're calling toString twice
You might be creating a simple field containing '.' in it. If the field name contains '.', make sure to create the intermediate ObjectFields. Make sure field name doesn't have any indexes or Path.ANY

This code assumes a lot of things about the context:

Path objectClassFieldPath = ldapNameTranslator.translateAttributeName(LdapConstant.ATTRIBUTE_OBJECT_CLASS);

^^^^ This assumes objectClassfieldPath can be a multi-level field

if(!fields.has(objectClassFieldPath.toString())){

^^^^ This assumes objectClassFieldName does not have '.', contradicts with the line above

fields.addNew(new ArrayField(objectClassFieldName, new SimpleArrayElement(StringType.TYPE)));

^^^ You might be creating a field with '.' in the name, or an array index in the name

TranslatorFromJson:

#100: dead code. resolve() throws exception if it can't resolve.

ResultTranslator:
#90: why path.matches? there are no patterns here

nitpicking {
I don't like this:
if(LightblueUtil.isFieldObjectType(fieldPath.toString())

This is much more obvious:
if(LDAPConstants.OBJECT_TYPE.equals(fieldPath.toString()))
}

LdapMetadata:

you are keeping a string->string map, which is quick during initialization, and keep using toString() and new Path() which is slower during actual runtime. Instead, keep two maps, one string->path, another path->string, set them up both during initialization (done once) and use trivial lookups for translate functions.

dcrissman · 2015-01-30T21:41:34Z

lightblue-ldap-crud/src/main/java/com/redhat/lightblue/crud/ldap/LdapMetadataListener.java

I don't really love this implementation, I am open to suggestions if a better way can be found.

FieldTreeNode dnNode=null;
try {
dnNode=md.resolve(dnFieldPath());
} catch (Error e) {
dnNode=addFieldToParent ...
}
if(!dnNode.getType() instanceof StringType)
{
throw ...
}

On Fri, Jan 30, 2015 at 2:41 PM, Dennis Crissman [email protected]
wrote:

In
lightblue-ldap-crud/src/main/java/com/redhat/lightblue/crud/ldap/LdapMetadataListener.java
#47 (comment)
:

LdapFieldNameTranslator ldapNameTranslator = LdapCrudUtil.getLdapFieldNameTranslator(md);

//TODO: check for array index or Path.any

Path dnFieldPath = ldapNameTranslator.translateAttributeName(LdapConstant.ATTRIBUTE_DN);

try{

FieldTreeNode dnNode = md.resolve(dnFieldPath);

if((!(dnNode instanceof SimpleField)) || (!(dnNode.getType() instanceof StringType))){

throw Error.get(MetadataConstants.ERR_FIELD_WRONG_TYPE, dnNode.getFullPath().toString());

}

}

catch(Error e){

if(e.getErrorCode().equals(MetadataConstants.ERR_FIELD_WRONG_TYPE)){

throw e;

}

addFieldToParent(md, dnFieldPath, new SimpleField(dnFieldPath.getLast(), StringType.TYPE));

}

I don't really love this implementation, I am open to suggestions if a
better way can be found.

—
Reply to this email directly or view it on GitHub
https://github.com/lightblue-platform/lightblue-ldap/pull/47/files#r23874595
.

dcrissman · 2015-01-30T21:57:34Z

MetadataConstants.java

I am fine with using the already defined error codes. I am however confused about what many of them mean. There are a lot of them, and it is not always obvious what code should be used when. I honestly kind of guessed on most of the ones I used. Maybe some in-line comments or documentation would be useful here?

#90: why path.matches? there are no patterns here

This was a misunderstanding on my part. I switch to the equals method.

if(LDAPConstants.OBJECT_TYPE.equals(fieldPath.toString()))

If we are going to do that, then I suggest we add OBJECT_TYPE as a constant somewhere in core. My intention was that LightblueUtil could be considered for inclusion in core eventually, if it made sense. Then build some common mechanisms for determining if a Field was "auto generated". This could be useful for plugins that do not actually store object_type or other generated fields in the underlying datastore.

you are keeping a string->string map, which is quick during initialization, and keep using toString() and new Path() which is slower during actual runtime. Instead, keep two maps, one string->path, another path->string, set them up both during initialization (done once) and use trivial lookups for translate functions.

I agree that the Path object should be stored in the Map and not the String representation. So I made that change. I would like to discuss the second part of your request further.

coveralls · 2015-01-30T23:02:26Z

Coverage increased (+5.34%) to 54.11% when pulling 6d1fefd on dcrissman:issue30-support-object-field into f67a2c7 on lightblue-platform:master.

bserdar · 2015-01-30T23:37:17Z

I agree: need more documentation about what error code means what, and what
additional information to expect.

Why OBJECT_TYPE as a constant in core? That's LDAP specific, put it in an
LDAPConstants file. You can move LightblueUtil to core, but remove the LDAP
specifics from it.

You got stuck in this idea of "auto generated". They are fields
specifically recognized by the particular back end. _id is recognized by
mongo backend, and it deals with it. ObjectType is the same for LDAP. There
is nothing special about being it "auto-generated". LDAP knows that it
exists, and that's that, you don't need to know more than that.

On Fri, Jan 30, 2015 at 2:57 PM, Dennis Crissman [email protected]
wrote:

MetadataConstants.java

I am fine with using the already defined error codes. I am however
confused about what many of them mean. There are a lot of them, and it is
not always obvious what code should be used when. I honestly kind of
guessed on most of the ones I used. Maybe some in-line comments or
documentation would be useful here?

#90: why path.matches? there are no patterns here

This was a misunderstanding on my part. I switch to the equals method.

if(LDAPConstants.OBJECT_TYPE.equals(fieldPath.toString()))

If we are going to do that, then I suggest we add OBJECT_TYPE as a
constant somewhere in core. My intention was that LightblueUtil could be
considered for inclusion in core eventually, if it made sense. Then build
some common mechanisms for determining if a Field was "auto generated".
This could be useful for plugins that do not actually store object_type or
other generated fields in the underlying datastore.

you are keeping a string->string map, which is quick during
initialization, and keep using toString() and new Path() which is slower
during actual runtime. Instead, keep two maps, one string->path, another
path->string, set them up both during initialization (done once) and use
trivial lookups for translate functions.

I agree that the Path object should be stored in the Map and not the
String representation. So I made that change. I would like to discuss the
second part of your request further.

—
Reply to this email directly or view it on GitHub
#47 (comment)
.

dcrissman · 2015-02-02T14:24:24Z

Why OBJECT_TYPE as a constant in core? That's LDAP specific, put it in an LDAPConstants file. You can move LightblueUtil to core, but remove the LDAP specifics from it.

I though objectType was Lightblue's way of identifying the entity? I think you might be confusing objectType with LDAP's objectClass.

You got stuck in this idea of "auto generated". They are fields specifically recognized by the particular back end. _id is recognized by mongo backend, and it deals with it. ObjectType is the same for LDAP. There is nothing special about being it "auto-generated". LDAP knows that it exists, and that's that, you don't need to know more than that.

I guess what I mean by auto-generated, is the objectType field and array count fields (prefixed by '#'). These are lightblue entity fields that need to exist, but don't necessarily physically exist in a particular backend. For the purposes of LDAP, I need to filter them out on a modifying operation and create them on a find operation. I understand that certain backends will have their own auto-generated fields, such as _id or objectClass and that those specific backends will need to handle them appropriately.

bserdar · 2015-02-02T14:32:42Z

Yes, I am confusing the two. objectType in code is ok, if it is not already
there. It might be already there somewhere, not probably in util.

On Mon, Feb 2, 2015 at 7:24 AM, Dennis Crissman [email protected]
wrote:

Why OBJECT_TYPE as a constant in core? That's LDAP specific, put it in an
LDAPConstants file. You can move LightblueUtil to core, but remove the LDAP
specifics from it.

I though objectType was Lightblue's way of identifying the entity? I think
you might be confusing objectType with LDAP's objectClass.

You got stuck in this idea of "auto generated". They are fields
specifically recognized by the particular back end. _id is recognized by
mongo backend, and it deals with it. ObjectType is the same for LDAP. There
is nothing special about being it "auto-generated". LDAP knows that it
exists, and that's that, you don't need to know more than that.

I guess what I mean by auto-generated, is the objectType field and array
count fields (prefixed by '#'). These are lightblue entity fields that need
to exist, but don't necessarily physically exist in a particular backend.
For the purposes of LDAP, I need to filter them out on a modifying
operation and create them on a find operation. I understand that certain
backends will have their own auto-generated fields, such as _id or
objectClass and that those specific backends will need to handle them
appropriately.

—
Reply to this email directly or view it on GitHub
#47 (comment)
.

dcrissman · 2015-02-02T14:50:54Z

Back to the LdapMetadata map discussion. I switched it over to a BiMap (http://google-collections.googlecode.com/svn/trunk/javadoc/index.html?com/google/common/collect/BiMap.html). I think this solves my concern of managing two Maps, and your concern about the cost of iterating over the values. Let me know what you think.

coveralls · 2015-02-02T14:54:22Z

Coverage increased (+5.28%) to 54.05% when pulling 8ebf356 on dcrissman:issue30-support-object-field into f67a2c7 on lightblue-platform:master.

bserdar · 2015-02-02T14:56:54Z

If you already had google.common package included, it's ok to use it. I'd
not include it just because of this though.

On Mon, Feb 2, 2015 at 7:50 AM, Dennis Crissman [email protected]
wrote:

Back to the LdapMetadata map discussion. I switched it over to a BiMap (
http://google-collections.googlecode.com/svn/trunk/javadoc/index.html?com/google/common/collect/BiMap.html).
I think this solves my concern of managing two Maps, and your concern about
the cost of iterating over the values. Let me know what you think.

—
Reply to this email directly or view it on GitHub
#47 (comment)
.

dcrissman · 2015-02-02T15:04:25Z

It is a transient dependency of lightblue-core-config

dcrissman · 2015-02-02T16:03:37Z

Ready for review.

coveralls · 2015-02-02T16:09:07Z

Coverage increased (+5.4%) to 54.17% when pulling da8d967 on dcrissman:issue30-support-object-field into f67a2c7 on lightblue-platform:master.

coveralls · 2015-02-02T19:48:43Z

Coverage increased (+4.81%) to 53.58% when pulling dfd0c0d on dcrissman:issue30-support-object-field into f67a2c7 on lightblue-platform:master.

Fixes #30: support object field

dcrissman added 2 commits January 27, 2015 15:17

couple small changes to unit tests

7c5d598

fixes lightblue-platform#30 and lightblue-platform#42 - support objec…

334cde5

…t fields and use Paths over fieldName fix crud errors so that they actually are sent to the client

dcrissman added in progress ready for review and removed in progress labels Jan 27, 2015

dcrissman added in progress and removed ready for review labels Jan 27, 2015

LdapFieldNameTranslator#translateAttributeName now returns a Path

459ff20

dcrissman added ready for review and removed in progress labels Jan 28, 2015

jewzaam changed the title ~~Issue30 support object field~~ Fixes #30: support object field Jan 28, 2015

dcrissman added 6 commits January 28, 2015 13:15

use ctx error over document error

e837644

simplify EntryBuilder method signatures

ce8b284

add unit test for unsupported objectarrays

5beb0c5

use Path copy utilities

1c75553

Add error push/pop to TranslatorFromJson

2570e4d

use Error push/pop and throw Errors over Exceptions

4097c7e

changes based on feedback

6d1fefd

dcrissman reviewed Jan 30, 2015
View reviewed changes

use BiMap over iterating of Map values to find key

8ebf356

cleanup LdapMetadataListener

da8d967

adjustments to better utilize the tools provided by core

dfd0c0d

bserdar added a commit that referenced this pull request Feb 2, 2015

Merge pull request #47 from dcrissman/issue30-support-object-field

eb252cc

Fixes #30: support object field

bserdar merged commit eb252cc into lightblue-platform:master Feb 2, 2015

bserdar removed the ready for review label Feb 2, 2015

dcrissman added the ready for review label Feb 2, 2015

dcrissman deleted the issue30-support-object-field branch February 2, 2015 19:54

Fixes #30: support object field #47

Fixes #30: support object field #47

Uh oh!

Conversation

dcrissman commented Jan 27, 2015

Uh oh!

coveralls commented Jan 27, 2015

Uh oh!

dcrissman commented Jan 27, 2015

Uh oh!

dcrissman commented Jan 27, 2015

Uh oh!

dcrissman commented Jan 28, 2015

Uh oh!

coveralls commented Jan 28, 2015

Uh oh!

bserdar commented Jan 28, 2015

Uh oh!

bserdar commented Jan 28, 2015

Uh oh!

coveralls commented Jan 28, 2015

Uh oh!

dcrissman commented Jan 28, 2015

Uh oh!

bserdar commented Jan 29, 2015

Uh oh!

dcrissman Jan 30, 2015

Choose a reason for hiding this comment

Uh oh!

bserdar Jan 30, 2015

Choose a reason for hiding this comment

Uh oh!

dcrissman commented Jan 30, 2015

Uh oh!

coveralls commented Jan 30, 2015

Uh oh!

bserdar commented Jan 30, 2015

Uh oh!

dcrissman commented Feb 2, 2015

Uh oh!

bserdar commented Feb 2, 2015

Uh oh!

dcrissman commented Feb 2, 2015

Uh oh!

coveralls commented Feb 2, 2015

Uh oh!

bserdar commented Feb 2, 2015

Uh oh!

dcrissman commented Feb 2, 2015

Uh oh!

dcrissman commented Feb 2, 2015

Uh oh!

coveralls commented Feb 2, 2015

Uh oh!

coveralls commented Feb 2, 2015

Uh oh!

Uh oh!