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

Move ConnectionIdentifier to util package. #1556

Merged
merged 4 commits into from May 4, 2014

Conversation

Projects
None yet
4 participants
@eltimn
Copy link
Member

eltimn commented May 3, 2014

I also replaced the use of MongoIdentifier with ConnectionIdentifier.

Then, I added RecordRules.fieldName that takes a ConnectionIdentifier as an argument. I had to add def connectionIdentifier to MetaRecord for this to work.

Replaces #1550

One question: DefaultConnectionIdentifier.jndiName was defined as a var. Does anyone know if that was intentional? I changed it to val here.

After I did this I realized we don't necessarily need to deprecate db.ConnectionIdentifier. We could just leave the type alias the same way mapper is.

@fmpwizard

This comment has been minimized.

Copy link
Member

fmpwizard commented May 3, 2014

👍

DB.rollback(DefaultConnectionIdentifier)
tryo(DB.use(util.DefaultConnectionIdentifier) {c =>
DB.appendPostTransaction(util.DefaultConnectionIdentifier, m.f _)
DB.rollback(util.DefaultConnectionIdentifier)

This comment has been minimized.

@Shadowfiend

Shadowfiend May 4, 2014

Member

It seems like it would have been cleaner for this entire file to just import DefaultConnectionIdentifier from util instead of replacing everything?

This comment has been minimized.

@Shadowfiend

Shadowfiend May 4, 2014

Member

You can address in a followup PR if you agree; I'll go ahead and merge this guy as is.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented May 4, 2014

👍

Shadowfiend added a commit that referenced this pull request May 4, 2014

Merge pull request #1556 from lift/tcn_issue_1555
Move ConnectionIdentifier to util package.

I also replaced the use of MongoIdentifier with ConnectionIdentifier.

Then, I added RecordRules.fieldName that takes a ConnectionIdentifier as an
argument. I had to add def connectionIdentifier to MetaRecord for this to work.

@Shadowfiend Shadowfiend merged commit 06c5b60 into master May 4, 2014

@Shadowfiend Shadowfiend deleted the tcn_issue_1555 branch May 4, 2014

@fmpwizard fmpwizard added this to the 2.6-M4 milestone Jun 7, 2014

@d6y

This comment has been minimized.

Copy link
Member

d6y commented Sep 7, 2014

I suspect DefaultConnectionIdentifier.jndiName was a var to allow users to use any JNDI name they want (or already have, e.g., jndi/legacy-db). The var is set in examples across the Wiki and elsewhere.

Often it is checked via....

def jndiJdbcConnAvailable_? : Boolean = jndiConnection(DefaultConnectionIdentifier).isDefined

in DB.scala:100

This suggests to me we need a version of jndiJdbcConnAvailable_? that takes a ConnectionIdentifier as an argument.

@eltimn

This comment has been minimized.

Copy link
Member Author

eltimn commented Sep 8, 2014

If a different jndiName is needed, wouldn't the best solution be to create a custom ConnectionIdentifier?

I failed to find anything in the wiki showing an example of setting the jndiName var. Do you know where that is?

In any case, I don't think it'd be too late to change it back. Also, although I've never used the DB stuff, I would agree that a version of jndiJdbcConnAvailable_? that takes a ConnectionIdentifier as an argument would be a good idea.

@d6y

This comment has been minimized.

Copy link
Member

d6y commented Sep 13, 2014

It looks like there's only one mention on the wiki, right at the end of the page on PostgresSQL. That's not a problem. My concern is only that it's a way of working that's already in use, and I'd like to understand the impact for users to change.

I have a couple of app that uses the JNDI thing: for Mapper and Squeryl . I'll go through a port exercise and see what we a user needs to do to get to the custom ConnectionIdentifier way of working. I'll report back here on the steps.

That aside: it's a breaking change. Deprecated and scheduled for termination in 3.0?

@eltimn

This comment has been minimized.

Copy link
Member Author

eltimn commented Sep 15, 2014

The var to val change was not deprecated, I just made the change because it didn't make much sense to me to use a mutable var here. I tend to avoid that when there's no compelling reason to use one.

Since nobody responded to my question on this PR (to be fair it was kind of buried) I just left it in.

At this point we should probably just revert that change, I just hope that doesn't mess up the 2.6 RC cycle.

@fmpwizard

This comment has been minimized.

Copy link
Member

fmpwizard commented Sep 15, 2014

we can do another RC build to get this in

@d6y

This comment has been minimized.

Copy link
Member

d6y commented Sep 17, 2014

BTW, I was able to get my Mapper app converted over to use a custom ConnectionIdentifier.

For future reference/migration, it looks like the steps are:

  • Create a new ConnectionIdentifier (case object MyConnectionIdentifier extends ConnectionIdenfitier...)
  • In each MetaMapper, override def dbDefaultConnectionIdentifier = MyConnectionIdentifier.
  • If you use schemify, include the connection ID. E.g. Schemifier.schemify(true, Schemifier.infoF _, MyConnectionIdentifier, Table1....etc)
  • If you're using S.addAround(DB.buildLoanWrapper), use the version that lets you set the connection identifier: S.addAround(DB.buildLoanWrapper(MyConnectionIdentifier :: Nil))
@fmpwizard

This comment has been minimized.

Copy link
Member

fmpwizard commented Sep 17, 2014

Thanks for posting them Richard, I didn't want them to be lost in the PR so I added them here https://www.assembla.com/spaces/liftweb/wiki/Cool_Tips#custom_connectionidenfitier .

I think it may be useful to add them a comments to the code if we go and deprecate it

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Sep 17, 2014

How common is this flow? Is it common enough that we should have some easier way to do this? Dunno if we want to move the discussion to the ML.

@eltimn

This comment has been minimized.

Copy link
Member Author

eltimn commented Sep 22, 2014

I started a discussion on the ML

@d6y does DB.jndiJdbcConnAvailable_? need to be updated if we go ahead with this change?

@d6y

This comment has been minimized.

Copy link
Member

d6y commented Sep 23, 2014

@eltimn yeah, if we do make it a val, we should allow users to test their customer connection identifiers. Something like: https://gist.github.com/d6y/194cf56a0f54105f6604

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment