Validate Variable key is a non-empty string in get() and set()

[bramhanandlingala]

Description

While looking into #69595, I checked Variable.set() and Variable.get() in airflow-core/src/airflow/models/variable.py, since they're two of the most commonly used public APIs in Airflow — called directly in DAG code all the time.

Root cause:
Neither function validated the key argument before doing anything with it. In Variable.set(), the key flowed straight through to a database write. The key column is declared NOT NULL with no nullable=True, so Variable.set(None, "x") reached the database before failing, surfacing a raw sqlalchemy.exc.IntegrityError with a stack trace that doesn't say what actually went wrong. Variable.set("", "x") was worse — an empty string satisfies NOT NULL, so it silently succeeded and created a variable with an unusable empty-string key.

Variable.get() had a milder version of the same gap. Variable.get(None) doesn't crash, but it queries the database for a variable literally named None, finds nothing, and raises KeyError("Variable None does not exist.") — a message that reads like there's a real variable you're missing, not that the key itself was invalid.

Fix:
Added the same check to the top of both methods, before any database or Task-SDK redirect logic runs:

if not key or not isinstance(key, str):
    raise ValueError("Variable key must be a non-empty string")

This catches all three cases the issue describes — None, empty string, and non-string values — with one consistent, descriptive error instead of a database exception or a misleading KeyError.

Didn't need to touch setdefault() separately since it calls get() internally as its first line, so it inherits the same validation automatically. Added a test to confirm that rather than just assuming it.

related: #69595

[bramhanandlingala]

thanks @potiuk

@bugraoz93, @choo121600, @ephraimbuddy, @henry3260, @jason810496, @rawwar @kaxil @Lee-W
Request You All, please review and Merge as need for this fix

[bramhanandlingala]

@bugraoz93, @choo121600, @ephraimbuddy, @henry3260, @jason810496, @rawwar @kaxil @Lee-W @shahar1

request You all please review and approve this PR for merge

[shahar1]

First, kind request for next times - please do not tag multiple maintainers in pull requests without ensuring that all are ok with it. Maintainers here have their own priorities - so please consider it and respect their time. If you don't get an answer within a reasonable time, you may post on either of the Slack channels (#new-contributors / #contributors). Tagging 10 maintainers in the PR once a week is not the appropriate way to do so (also, some of the maintainers might not be domain experts of this specific area).

Also, please note that the original issue was marked as an AI-slop and closed (author spammed general Python advices as issues and PRs with no good use), so it already brings the question of real necessity on the table.

---

Putting the above aside, let's get to the point - the change itself doesn't address the problem it claims to solve (disclaimer - the following were drafted by AI, validated by myself):

1. Wrong layer: In Airflow 3, Dag authors should use airflow.sdk.Variable - airflow.models.Variable is the deprecated compat path that warns and redirects to the SDK in task context. This PR validates only the deprecated entry point, while airflow.sdk.Variable.set(None, ...) / .get(None) behave exactly as before. So the API we actually tell users to use gains nothing, and the two entry points now diverge.

2. Behavior change: Variable.get() misses raise KeyError, and except KeyError is a common user pattern. After this change get(None) raises ValueError instead - a user-facing contract change that can't ride in as a side effect (and would at least need a newsfragment).

3. Inconsistency: delete() is left unvalidated, so the class ends up half-validated.
   The actual "problem" is also thin: set(None) already fails loudly (ugly IntegrityError, but it fails), get(None) raises KeyError, and empty-string keys are already rejected by the API/UI on normal paths. That leaves a cosmetic error-message improvement on a deprecated path - not enough to justify the contract change.

---

Closing this one. If there's a real user report of hitting this, key validation could be reconsidered - but in the Task SDK class, applied consistently across get/set/delete.