Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type inference on anonymous objects #1761

Closed
gmarz opened this issue Jan 27, 2016 · 7 comments
Closed

Type inference on anonymous objects #1761

gmarz opened this issue Jan 27, 2016 · 7 comments

Comments

@gmarz
Copy link
Contributor

gmarz commented Jan 27, 2016

Currently, if an anonymous object is indexed without specifying a type:

client.Index(new { foo = "bar" }, i => i.Index("foo"));

the type is inferred as <>f__anonymoustype0`1

Should we throw an exception instead?

@gmarz gmarz added the Vote label Jan 27, 2016
@russcam
Copy link
Contributor

russcam commented Jan 27, 2016

All anonymous type type names start with <> (along with a few other characteristics).

Should we infer type to be object?

@gmarz
Copy link
Contributor Author

gmarz commented Jan 27, 2016

All anonymous type type names start with <> (along with a few other characteristics).

Right, but it seems weird to use that name as a type name in ES.

object is better, and technically follows suit with our inference rules...but it still seems a little unassuming to me.

Also, another potential issue- when an index isn't specified:

client.Index(new { foo = "bar" });

or

client.Index(new Foo { Foo = "bar" });

The index isn't inferred and we get an exception when dispatching the call ("not enough information"). Shouldn't we be falling back to the default index?

@russcam
Copy link
Contributor

russcam commented Jan 27, 2016

I think making the assumption that anonymous types are of type object is reasonable since one general usage is to pass them to methods that accept a parameter of type object e.g. MVC routing, html attributes on HtmlHelper methods.

As for the other issue - yes, I think we should be using the default index here as per NEST 1.x 👍

@russcam
Copy link
Contributor

russcam commented Jan 29, 2016

NEST 1.x uses the anonymous type name e.g.

<>f__anonymoustype0`1

so 2.x is consistent with 1.x right now

@russcam
Copy link
Contributor

russcam commented Feb 1, 2016

Throw exception on known invalid type and index names

@russcam
Copy link
Contributor

russcam commented Feb 2, 2016

issue #9059 on input validation rules for Elasticsearch is still open in regards to valid and invalid characters in directory/filenames.

From source, Elasticsearch does not allow the following characters in a file name i.e. index name currently

'\\', '/', '*', '?', '"', '<', '>', '|', ' ', ','

and there are also some checks for invalid characters in type names

if (mapper.type().length() == 0) {
    throw new InvalidTypeNameException("mapping type name is empty");
}
if (mapper.type().length() > 255) {
    throw new InvalidTypeNameException("mapping type name [" + mapper.type() + "] is too long; limit is length 255 but was [" + mapper.type().length() + "]");
}
if (mapper.type().charAt(0) == '_') {
    throw new InvalidTypeNameException("mapping type name [" + mapper.type() + "] can't start with '_'");
}
if (mapper.type().contains("#")) {
    throw new InvalidTypeNameException("mapping type name [" + mapper.type() + "] should not include '#' in it");
}
if (mapper.type().contains(",")) {
    throw new InvalidTypeNameException("mapping type name [" + mapper.type() + "] should not include ',' in it");
}
if (mapper.type().equals(mapper.parentFieldMapper().type())) {
    throw new IllegalArgumentException("The [_parent.type] option can't point to the same type");
}
if (typeNameStartsWithIllegalDot(mapper)) {
    throw new IllegalArgumentException("mapping type name [" + mapper.type() + "] must not start with a '.'");
}

But given that the issue on validation is still open and being discussed, I'm suggesting we keep the current behaviour and close this issue for the time being. The exception messages that are returned have a clear description of what the problem is.

Thoughts, @gmarz and @Mpdreamz ?

@gmarz
Copy link
Contributor Author

gmarz commented Feb 2, 2016

+1 I think we can close this

@russcam russcam closed this as completed Feb 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants