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

Version 1.5 Migration Guide #1337

Closed
ctmay4 opened this issue Apr 15, 2019 · 12 comments
Closed

Version 1.5 Migration Guide #1337

ctmay4 opened this issue Apr 15, 2019 · 12 comments
Assignees
Labels
Milestone

Comments

@ctmay4
Copy link

ctmay4 commented Apr 15, 2019

Is your feature request related to a problem? Please describe.
I noticed 1.5 was released today and I want to do some testing. There were tons of deprecation warnings and log messages. Some were obvious fixes, others are not so much.

Describe the solution you'd like
A migration guide explaining common changes that are needed.

Additional context
Here are a couple of cases:

  1. I get the following log messages now:

    Embedded index generation is being removed from 2.0.  Please migrate any index definitions 
    you need to the parent entity to ensure these indexes continue to be generated in future 
    versions.
    

    I had two embedded entities with @Index annotations. I removed them and I still get this warning. How is this supposed to be fixed?

  2. I have many warnings that look like this:

    Warning:(220, 58) java: asList() in dev.morphia.query.Query has been deprecated
    

    The Javadoc states to use find(FindOptions options). That method returns a MongoCursor<T>. I have 100's of methods that return a List and I see no builtin non-deprecated way to get it. Do I need to loop over the cursor and build a List myself? It feels like I'm missing something.

There are others that I'm still working through, but I just wanted to proved a couple of examples. Is there anything I can take a look at?

@evanchooly
Copy link
Member

First, thanks for the quick up take. I actually started one but I just realized it's for 1.5 -> 2.0. The deprecations should all list the recommended approaches.

As for the embedded index warnings, there's a setting on MapperOptions to turn off indexing of embedded types. Setting that should clear up that warning for you. I'll start a migration guide for 1.5, though. That was a bit of an oversight on my part.

@evanchooly evanchooly self-assigned this Apr 15, 2019
@ctmay4
Copy link
Author

ctmay4 commented Apr 15, 2019

Thanks for the quick response! The MapperOptions fix removed the log message. Awesome.

Any idea about the replacement for asList() to get a List from a Query? Is that really gone?

@evanchooly
Copy link
Member

That's the plan for now wrt asList(). I could be talked out of it but it comes up as a "bug" from time to time with OOME failures. Part of my 2.0 plan is to slim the API down to reduce the surface area. This is one I still vacillate over.

@ctmay4
Copy link
Author

ctmay4 commented Apr 15, 2019

That seems like an odd decision. For example, I have TONs of APIs that look like this:

public List<Entity> find(String param1, String param2) {
    Query<Entity> q = _ds.createQuery(Entity.class);
    q.field("param1").equal(param1);
    q.field("param2").equal(param2);

    return q.asList();
}

I just found MorphiaCursor.toList(). Is that going to stay? If so I can convert all my code to look like this:

public List<Entity> find(String param1, String param2) {
    Query<Entity> q = _ds.createQuery(Entity.class);
    q.field("param1").equal(param1);
    q.field("param2").equal(param2);

    return MorphiaCursor.toList(q.find());
}

If that is going to be there, I don't really see a difference in Query.toList. It's definitely your call though and I just want to give my 2 cents. Thanks again for all the work you do on this.

@evanchooly
Copy link
Member

Your 2 cents are exactly what I'm wanting. So thank you. I had to update a lot of the tests, too, which made me start rethinking it all. I'm still not 100% sold on it. I want the smaller API but ...

@evanchooly evanchooly changed the title Version 1.5 Migraiton Guide Version 1.5 Migration Guide Apr 15, 2019
@ctmay4
Copy link
Author

ctmay4 commented Apr 15, 2019

I see your point. I'm going to convert my code to use MorphiaCursor.toList() to get rid of the deprecations for now. I'm just trying to get a clean compile.

@evanchooly
Copy link
Member

I'm going to do an under the radar .1 release to fix some documentation gaps and track some code quality check updates (spotbugs, checkstyle, etc.). And I think i'll add a toList() to replace that asList(). Essentially a rename to make that method name idiomatic. At the end of the day, it's a super useful method used by virtually everybody. I'll deal with any OOME questions as they come up. :)

@ctmay4
Copy link
Author

ctmay4 commented Apr 16, 2019

Sounds great. Thanks! Is wasn't difficult to switch to MorphiaCursor.toList(q.find()) but that will definitely be cleaner.

@evanchooly
Copy link
Member

oh, ok. I apologize for the back and forth but I finally remembered the "why." if you look at asList() and asKeyList() there are multiple overloads for with and without options. this proliferation of overloads is what was causing all the heartburn. Another option is to return a MorphiaCursor rather than MongoCursor. I've been trying to avoid/reduce the amount of near-duplicative types between Morphia and the driver to simplify both the API and maintenance. But perhaps this situation warrants it.

@ctmay4
Copy link
Author

ctmay4 commented Apr 16, 2019

Can I ask one more question about the update? It's possible I was doing something wrong before, but I am seeing a different behavior with the new version I'm not sure I understand. Here is a code snippet:

Query<C> q = getDatastore().createQuery(_changelogClass);

q.field("version").equal(version);

// POINT A

// id searching
if (param.getDisplayId() != null) {
    List<Criteria> orCriteria = new ArrayList<>();

    orCriteria.add(q.criteria("adds.id").equal(param.getDisplayId()));
    orCriteria.add(q.criteria("deletes.id").equal(param.getDisplayId()));
    orCriteria.add(q.criteria("mods.id").equal(param.getDisplayId()));

    q.or(orCriteria.toArray(new Criteria[orCriteria.size()]));
}

// POINT B

In the code I am optionally adding an "or" block. I put the string representation of the query object below at two points in both versions

In 1.3.x:

POINT A:

{ query: { "version" : "latest" }  }

POINT B:

{
    query: {
        "version": "latest",
        "$or": [{
            "adds.id": "5cb5fa6f8d7bd65e8276cd48"
        }, {
            "deletes.id": "5cb5fa6f8d7bd65e8276cd48"
        }, {
            "mods.id": "5cb5fa6f8d7bd65e8276cd48"
        }]
    }
}

In 1.5.0:

POINT A:

{ query: { "version" : "latest" }  }

POINT B:

{
    {
        "$or": [{
            "adds.id": "5cb5fa6f8d7bd65e8276cd48"
        }, {
            "deletes.id": "5cb5fa6f8d7bd65e8276cd48"
        }, {
            "mods.id": "5cb5fa6f8d7bd65e8276cd48"
        }]
    }
}

In the new version the "or" overwrites what is in the query, whereas under 1.3 it added an "or" criteria. Is this a known change? I could write this in different ways, but I wanted to highlight that it now produces different results.

I can fix my code by changing it to look like this:

Query<C> q = getDatastore().createQuery(_changelogClass);

List<Criteria> andCriteria = new ArrayList<>();

andCriteria.add(q.criteria("version").equal(version));

// id searching
if (param.getDisplayId() != null) {
    andCriteria.add(q.or(
            q.criteria("adds.id").equal(param.getDisplayId()),
            q.criteria("deletes.id").equal(param.getDisplayId()),
            q.criteria("mods.id").equal(param.getDisplayId())
    ));
}

q.and(andCriteria.toArray(new Criteria[0]));

@evanchooly
Copy link
Member

I did do some work with that criteria building and it looks like perhaps I introduced a bug. if you could file a separate issue on that, i'll take a look tonight.

@ctmay4
Copy link
Author

ctmay4 commented Apr 16, 2019

Sure, no problem.

@evanchooly evanchooly added this to the 1.5.1 milestone Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants