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

support array type for #150 #294

Closed
wants to merge 1 commit into from
Closed

support array type for #150 #294

wants to merge 1 commit into from

Conversation

lirispp
Copy link

@lirispp lirispp commented May 1, 2018

Add Array type column for PostgreSQL.

@Tapac
Copy link
Contributor

Tapac commented May 4, 2018

Thank you for PR.
Could you please add some tests to ensure that arrays will work on every DBMS (AFAIK MySQL doesn't support ARRAY column type)?

@frellan
Copy link

frellan commented Jun 20, 2018

How is it going with this? 😄

@Drako
Copy link

Drako commented Aug 8, 2018

@lirispp @saibotma as @Tapac said, tests would be really nice to have. Yet I do not fully understand this "will work on every DBMS" mentality.

I don't exactly see, why it would be so bad to say "If you want to use this specific feature you need one of these DBMS..." Especcially as emulating the behaviour in software usually yields suboptimal results.

@MrPowerGamerBR
Copy link
Contributor

I agree with @Drako, why Exposed needs to be based on the DBMS that has less supported features?

What I ended up doing is copying @lirispp's code to my project while this PR isn't merged, you don't really need to fork Exposed to add array support to it, but having it on Exposed would be great.

@MrPowerGamerBR
Copy link
Contributor

MrPowerGamerBR commented Oct 12, 2018

By the way, this PR has issues with DAO objects.

	val stored = transaction(database) {
		SchemaUtils.createMissingTablesAndColumns(StoredMessages)
		StoredMessage.new(id) {
			authorId = 0
			channelId = 1
			content = "owo"
			createdAt = System.currentTimeMillis()
			storedAttachments = arrayOf("owo")
		}
	}

	transaction(database) {
		stored.storedAttachments = arrayOf("hello world")
	}

This causes an Exception in thread "main" java.lang.IllegalStateException: Array does not support for this database, to fix this, an

		if (value is Array<*>) {
			return value
		}

needs to be added to the valueFromDB method.

And, of course, Array does not support for this database is not correct, right? Shouldn't it be This DBMS doesn't support arrays?

@Savrov
Copy link

Savrov commented Jan 5, 2019

So will this branch merged to master?

@Tapac
Copy link
Contributor

Tapac commented Jan 7, 2019

@Drako, @MrPowerGamerBR , I just mean that if you add a feature to a framework which works with different DBMS you have to implement it for every DBMS where it's possible and throw Unsupported exception for others.
Otherwise, users who know what e.g. Arrays supported by Oracle will end up with unworking code.

Also, I see a lack of type-safety in provided operators (any/contains).

Adding specific features will be much easier after splitting Exposed to modules (see p.2 here)

@e5l e5l deleted the branch JetBrains:master June 5, 2023 07:00
@e5l e5l closed this Jun 5, 2023
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

Successfully merging this pull request may close these issues.

7 participants